The current quirk_amd_8131_ioapic() function sets a global
pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
system. This is safe but suboptimal, because there may be devices on
other buses not related to the AMD 8131 bridge, for which MSI would
work fine. As an example, see the end of this email for a lspci -t
from a real Opteron system that has PCI-X buses coming from an AMD
8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
it to be enabled.
I guess what we really should be doing is setting the dev->no_msi flag
for all devices below the AMD 8131 PCI-X bridge rather than turning
off MSI globally. Of course this is somewhat tricky, since a device
could be hotplugged onto a bus below the AMD 8131. Greg, any thoughts
about the proper way to use the driver model infrastructure to handle
this?
The other place that pci_msi_quirk is set is quirk_svw_msi(). I don't
know why MSI is turned off for that Serverworks chipset, but perhaps
the same change could be made, and the global pci_msi_quirk flag
killed off entirely.
Thanks,
Roland
-+-[80]-+-00.0 nVidia Corporation CK804 Memory Controller
| +-01.0 nVidia Corporation CK804 Memory Controller
| +-0a.0 nVidia Corporation CK804 Ethernet Controller
| \-0e.0-[81]----00.0 Mellanox Technologies MT25208 InfiniHost III Ex (Tavor compatibility mode)
+-[08]-+-0a.0-[09]--
| +-0a.1 Advanced Micro Devices [AMD] AMD-8131 PCI-X APIC
| +-0b.0-[0a]--+-04.0 Intel Corporation 82546EB Gigabit Ethernet Controller (Copper)
| | +-04.1 Intel Corporation 82546EB Gigabit Ethernet Controller (Copper)
| | \-09.0 Chelsio Communications Inc: Unknown device 000b
| \-0b.1 Advanced Micro Devices [AMD] AMD-8131 PCI-X APIC
\-[00]-+-00.0 nVidia Corporation CK804 Memory Controller
+-01.0 nVidia Corporation CK804 ISA Bridge
+-01.1 nVidia Corporation CK804 SMBus
+-02.0 nVidia Corporation CK804 USB Controller
+-02.1 nVidia Corporation CK804 USB Controller
+-04.0 nVidia Corporation CK804 AC'97 Audio Controller
+-06.0 nVidia Corporation CK804 IDE
+-07.0 nVidia Corporation CK804 Serial ATA Controller
+-08.0 nVidia Corporation CK804 Serial ATA Controller
+-09.0-[01]----05.0 Texas Instruments TSB43AB22/A IEEE-1394a-2000 Controller (PHY/Link)
+-0a.0 nVidia Corporation CK804 Ethernet Controller
+-0e.0-[02]--+-00.0 ATI Technologies Inc RV370 5B60 [Radeon X300 (PCIE)]
| \-00.1 ATI Technologies Inc: Unknown device 5b70
+-18.0 Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] HyperTransport Technology Configuration
+-18.1 Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Address Map
+-18.2 Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] DRAM Controller
+-18.3 Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Miscellaneous Control
+-19.0 Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] HyperTransport Technology Configuration
+-19.1 Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Address Map
+-19.2 Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] DRAM Controller
\-19.3 Advanced Micro Devices [AMD] K8 [Athlon64/Opteron] Miscellaneous Control
On Sat, Oct 22, 2005 at 03:14:34PM -0700, Roland Dreier wrote:
> The current quirk_amd_8131_ioapic() function sets a global
> pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
> system. This is safe but suboptimal, because there may be devices on
> other buses not related to the AMD 8131 bridge, for which MSI would
> work fine. As an example, see the end of this email for a lspci -t
> from a real Opteron system that has PCI-X buses coming from an AMD
> 8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
> fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
> it to be enabled.
>
> I guess what we really should be doing is setting the dev->no_msi flag
> for all devices below the AMD 8131 PCI-X bridge rather than turning
> off MSI globally. Of course this is somewhat tricky, since a device
> could be hotplugged onto a bus below the AMD 8131. Greg, any thoughts
> about the proper way to use the driver model infrastructure to handle
> this?
Perhaps the right thing to do is to change pad2 (in struct pci_bus) to
bus_flags and make bit 0 PCI_BRIDGE_FLAGS_NO_MSI ?
On Sat, Oct 22, 2005 at 05:32:20PM -0600, Matthew Wilcox wrote:
> On Sat, Oct 22, 2005 at 03:14:34PM -0700, Roland Dreier wrote:
> > The current quirk_amd_8131_ioapic() function sets a global
> > pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
> > system. This is safe but suboptimal, because there may be devices on
> > other buses not related to the AMD 8131 bridge, for which MSI would
> > work fine. As an example, see the end of this email for a lspci -t
> > from a real Opteron system that has PCI-X buses coming from an AMD
> > 8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
> > fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
> > it to be enabled.
> >
> > I guess what we really should be doing is setting the dev->no_msi flag
> > for all devices below the AMD 8131 PCI-X bridge rather than turning
> > off MSI globally. Of course this is somewhat tricky, since a device
> > could be hotplugged onto a bus below the AMD 8131. Greg, any thoughts
> > about the proper way to use the driver model infrastructure to handle
> > this?
>
> Perhaps the right thing to do is to change pad2 (in struct pci_bus) to
> bus_flags and make bit 0 PCI_BRIDGE_FLAGS_NO_MSI ?
Yeah, I can't think of any way to use the device tree to do this, so
this sounds as good a way as any.
thanks,
greg k-h
On Wed, Oct 26, 2005 at 03:51:40PM -0700, Greg KH wrote:
> On Sat, Oct 22, 2005 at 05:32:20PM -0600, Matthew Wilcox wrote:
> > Perhaps the right thing to do is to change pad2 (in struct pci_bus) to
> > bus_flags and make bit 0 PCI_BRIDGE_FLAGS_NO_MSI ?
It would be better to make it a bit field as in struct pci_dev.
- unsigned short pad2;
+ unsigned short no_msi:1;
> Yeah, I can't think of any way to use the device tree to do this, so
> this sounds as good a way as any.
Other pci_bus flags might be useful as well, for instance I thought
of bus->hotplug_bridge or something like that.
Ivan.
Matthew> Perhaps the right thing to do is to change pad2 (in
Matthew> struct pci_bus) to bus_flags and make bit 0
Matthew> PCI_BRIDGE_FLAGS_NO_MSI ?
Seems reasonable, but I'm still not sure how to implement this. Where
does this bit get set and propagated to secondary buses?
To give a somewhat pathological real-world example, Mellanox PCI-X
adapters have a PCI bridge in them; in other words, a single adapter
looks like:
0000:03:01.0 PCI bridge: Mellanox Technologies MT23108 PCI Bridge (rev a1) (prog-if 00 [Normal decode])
Flags: bus master, 66MHz, medium devsel, latency 64
Bus: primary=03, secondary=04, subordinate=04, sec-latency=68
Memory behind bridge: e8200000-e82fffff
Prefetchable memory behind bridge: 00000000ea800000-00000000f7f00000
Capabilities: [70] PCI-X bridge device.
0000:04:00.0 InfiniBand: Mellanox Technologies MT23108 InfiniHost (rev a1)
Subsystem: Mellanox Technologies MT23108 InfiniHost
Flags: bus master, fast Back2Back, 66MHz, medium devsel, latency 64, IRQ 185
Memory at e8200000 (64-bit, non-prefetchable) [size=1M]
Memory at ea800000 (64-bit, prefetchable) [size=8M]
Memory at f0000000 (64-bit, prefetchable) [size=128M]
Capabilities: [40] #11 [001f]
Capabilities: [50] Vital Product Data
Capabilities: [60] Message Signalled Interrupts: 64bit+ Queue=0/5 Enable-
Capabilities: [70] PCI-X non-bridge device.
That means the NO_MSI flag still needs to get propagated from the 8131
bridge to the Mellanox bridge, and that needs to cause no_msi to get
set on the actual device.
Also, if someone hot-plugged such an adapter into a bus below an AMD
8131 host bridge (I believe eg Sun V40Zs have hot-pluggable slots like
that), then the NO_MSI flag still needs to get propagated from the
8131 bridge to the Mellanox bridge and set no_msi on the final device.
Where in the PCI driver code is the right place to handle all this (I
hope by writing the code only once)?
- R.
On Thu, Oct 27, 2005 at 08:08:45AM -0700, Roland Dreier wrote:
> Matthew> Perhaps the right thing to do is to change pad2 (in
> Matthew> struct pci_bus) to bus_flags and make bit 0
> Matthew> PCI_BRIDGE_FLAGS_NO_MSI ?
>
> Seems reasonable, but I'm still not sure how to implement this. Where
> does this bit get set and propagated to secondary buses?
We can propagate it to secondary busses in pci_alloc_child_bus().
We inherit parent->ops and parent->sysdata at this point, we can also
inherit parent->whatever_flags_we_like.
Setting it from the quirk is a bit more yucky. I *think* we're going
to have to walk the PCI tree, given that it's a FIXUP_FINAL. Maybe it
needs to not be a FIXUP_FINAL ... a FIXUP_HEADER might get it set early
enough for it to propagate through that mechanism.
On Thu, Oct 27, 2005 at 08:08:45AM -0700, Roland Dreier wrote:
> Matthew> Perhaps the right thing to do is to change pad2 (in
> Matthew> struct pci_bus) to bus_flags and make bit 0
> Matthew> PCI_BRIDGE_FLAGS_NO_MSI ?
>
> Seems reasonable, but I'm still not sure how to implement this. Where
> does this bit get set and propagated to secondary buses?
Does it have to be propagate to secondary busses?
Can't the MSI init code walk up the tree until it hits a root node?
> To give a somewhat pathological real-world example, Mellanox PCI-X
> adapters have a PCI bridge in them; in other words, a single adapter
> looks like:
...
> Also, if someone hot-plugged such an adapter into a bus below an AMD
> 8131 host bridge (I believe eg Sun V40Zs have hot-pluggable slots like
> that), then the NO_MSI flag still needs to get propagated from the
> 8131 bridge to the Mellanox bridge and set no_msi on the final device.
>
> Where in the PCI driver code is the right place to handle all this (I
> hope by writing the code only once)?
I expect this could be contained in msi.c.
ie changes to msi_init(), pci_enable_msi(), msi_capability_init().
The flag would have to be set by whatever code claims the AMD 8131
chip.
hth,
grant
Quoting r. Roland Dreier <[email protected]>:
> Subject: AMD 8131 and MSI quirk
>
> The current quirk_amd_8131_ioapic() function sets a global
> pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
> system. This is safe but suboptimal, because there may be devices on
> other buses not related to the AMD 8131 bridge, for which MSI would
> work fine. As an example, see the end of this email for a lspci -t
> from a real Opteron system that has PCI-X buses coming from an AMD
> 8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
> fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
> it to be enabled.
The following should do this IMO. Roland, could you test this patch please?
---
It turns out AMD 8131 quirk only affects MSI for devices behind the 8131 bridge.
Handle this by adding a flags field in pci_bus, inherited from parent to child.
Signed-off-by: Michael S. Tsirkin <[email protected]>
Index: linux-2.6.16-rc2/drivers/pci/msi.c
===================================================================
--- linux-2.6.16-rc2.orig/drivers/pci/msi.c 2006-02-14 17:09:23.000000000 +0200
+++ linux-2.6.16-rc2/drivers/pci/msi.c 2006-02-14 18:14:00.000000000 +0200
@@ -699,6 +699,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;
+
temp = dev->irq;
if ((status = msi_init()) < 0)
@@ -924,6 +927,9 @@ int pci_enable_msix(struct pci_dev* dev,
if (!pci_msi_enable || !dev || !entries)
return -EINVAL;
+ if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
+ return -EINVAL;
+
if ((status = msi_init()) < 0)
return status;
Index: linux-2.6.16-rc2/drivers/pci/probe.c
===================================================================
--- linux-2.6.16-rc2.orig/drivers/pci/probe.c 2006-02-14 17:09:23.000000000 +0200
+++ linux-2.6.16-rc2/drivers/pci/probe.c 2006-02-14 17:58:17.000000000 +0200
@@ -347,6 +347,7 @@ pci_alloc_child_bus(struct pci_bus *pare
child->parent = parent;
child->ops = parent->ops;
child->sysdata = parent->sysdata;
+ child->bus_flags = parent->bus_flags;
child->bridge = get_device(&bridge->dev);
child->class_dev.class = &pcibus_class;
Index: linux-2.6.16-rc2/drivers/pci/quirks.c
===================================================================
--- linux-2.6.16-rc2.orig/drivers/pci/quirks.c 2006-02-14 17:09:23.000000000 +0200
+++ linux-2.6.16-rc2/drivers/pci/quirks.c 2006-02-14 17:58:17.000000000 +0200
@@ -575,8 +575,11 @@ static void __init quirk_amd_8131_ioapic
{
unsigned char revid, tmp;
- pci_msi_quirk = 1;
- printk(KERN_WARNING "PCI: MSI quirk detected. pci_msi_quirk set.\n");
+ 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;
Index: linux-2.6.16-rc2/include/linux/pci.h
===================================================================
--- linux-2.6.16-rc2.orig/include/linux/pci.h 2006-02-14 17:09:24.000000000 +0200
+++ linux-2.6.16-rc2/include/linux/pci.h 2006-02-14 17:58:17.000000000 +0200
@@ -95,6 +95,11 @@ enum pci_channel_state {
pci_channel_io_perm_failure = (__force pci_channel_state_t) 3,
};
+typedef unsigned short __bitwise pci_bus_flags_t;
+enum pci_bus_flags {
+ PCI_BUS_FLAGS_NO_MSI = (pci_bus_flags_t) 1,
+};
+
/*
* The pci_dev structure is used to describe PCI devices.
*/
@@ -203,7 +208,7 @@ struct pci_bus {
char name[48];
unsigned short bridge_ctl; /* manage NO_ISA/FBB/et al behaviors */
- unsigned short pad2;
+ pci_bus_flags_t bus_flags; /* Inherited by child busses */
struct device *bridge;
struct class_device class_dev;
struct bin_attribute *legacy_io; /* legacy I/O for this bus */
--
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies
On Tue, Feb 14, 2006 at 06:52:22PM +0200, Michael S. Tsirkin wrote:
> The following should do this IMO. Roland, could you test this patch please?
Going a bit overboard on the type safety. Please, leave bus_flags as an
unsigned short so as not to bloat the pci_bus structure unnecessarily.
> +typedef unsigned short __bitwise pci_bus_flags_t;
> +enum pci_bus_flags {
> + PCI_BUS_FLAGS_NO_MSI = (pci_bus_flags_t) 1,
> +};
> +
> /*
> * The pci_dev structure is used to describe PCI devices.
> */
> @@ -203,7 +208,7 @@ struct pci_bus {
> char name[48];
>
> unsigned short bridge_ctl; /* manage NO_ISA/FBB/et al behaviors */
> - unsigned short pad2;
> + pci_bus_flags_t bus_flags; /* Inherited by child busses */
> struct device *bridge;
> Going a bit overboard on the type safety. Please, leave bus_flags as an
> unsigned short so as not to bloat the pci_bus structure unnecessarily.
Hmm:
> +typedef unsigned short __bitwise pci_bus_flags_t;
and:
> - unsigned short pad2;
> + pci_bus_flags_t bus_flags; /* Inherited by child busses */
This does make pci_bus_flags_t a short -- it just lets sparse catch
misuses of the enum values.
It's debatable whether it's worth the source obfuscation to let sparse
check this, since it seems rather unlikely that someone will screw it
up. But I don't see how it makes any difference in the generated
code; certainly there's no bloat (beyond the extra source code)
- R.
Michael> The following should do this IMO. Roland, could you test
Michael> this patch please?
I'll need to find a system with the required setup; unfortunately I
don't have one myself.
What's required is an Opteron motherboard with both an AMD 8131 PCI-X
bridge and a PCI Express slot (typically Nvidia Nforce4), and an
MSI-capable device (such as a Mellanox HCA) on the PCIe bus. I know
that eg Tyan makes such a motherboard. Does anyone reading this have
a setup like that?
- R.
On Tue, 2006-02-14 at 18:52 +0200, Michael S. Tsirkin wrote:
> Quoting r. Roland Dreier <[email protected]>:
> > Subject: AMD 8131 and MSI quirk
> >
> > The current quirk_amd_8131_ioapic() function sets a global
> > pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
> > system. This is safe but suboptimal, because there may be devices on
> > other buses not related to the AMD 8131 bridge, for which MSI would
> > work fine. As an example, see the end of this email for a lspci -t
> > from a real Opteron system that has PCI-X buses coming from an AMD
> > 8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
> > fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
> > it to be enabled.
>
> The following should do this IMO. Roland, could you test this patch please?
>
> ---
>
> It turns out AMD 8131 quirk only affects MSI for devices behind the 8131 bridge.
> Handle this by adding a flags field in pci_bus, inherited from parent to child.
It seems like we have a way to turn of msi already (the no_msi bit in
the pci_dev structure). Does it make sense to just have the child bus
pci_dev structure inherit the no_msi bit from the parent's pci_dev
structure when doing an allocation, or does that unnecessarily remove
the msi capability for devices that may not need it?
Kristen
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
>
> Index: linux-2.6.16-rc2/drivers/pci/msi.c
> ===================================================================
> --- linux-2.6.16-rc2.orig/drivers/pci/msi.c 2006-02-14 17:09:23.000000000 +0200
> +++ linux-2.6.16-rc2/drivers/pci/msi.c 2006-02-14 18:14:00.000000000 +0200
> @@ -699,6 +699,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;
> +
> temp = dev->irq;
>
> if ((status = msi_init()) < 0)
> @@ -924,6 +927,9 @@ int pci_enable_msix(struct pci_dev* dev,
> if (!pci_msi_enable || !dev || !entries)
> return -EINVAL;
>
> + if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> + return -EINVAL;
> +
> if ((status = msi_init()) < 0)
> return status;
>
> Index: linux-2.6.16-rc2/drivers/pci/probe.c
> ===================================================================
> --- linux-2.6.16-rc2.orig/drivers/pci/probe.c 2006-02-14 17:09:23.000000000 +0200
> +++ linux-2.6.16-rc2/drivers/pci/probe.c 2006-02-14 17:58:17.000000000 +0200
> @@ -347,6 +347,7 @@ pci_alloc_child_bus(struct pci_bus *pare
> child->parent = parent;
> child->ops = parent->ops;
> child->sysdata = parent->sysdata;
> + child->bus_flags = parent->bus_flags;
> child->bridge = get_device(&bridge->dev);
>
> child->class_dev.class = &pcibus_class;
> Index: linux-2.6.16-rc2/drivers/pci/quirks.c
> ===================================================================
> --- linux-2.6.16-rc2.orig/drivers/pci/quirks.c 2006-02-14 17:09:23.000000000 +0200
> +++ linux-2.6.16-rc2/drivers/pci/quirks.c 2006-02-14 17:58:17.000000000 +0200
> @@ -575,8 +575,11 @@ static void __init quirk_amd_8131_ioapic
> {
> unsigned char revid, tmp;
>
> - pci_msi_quirk = 1;
> - printk(KERN_WARNING "PCI: MSI quirk detected. pci_msi_quirk set.\n");
> + 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;
> Index: linux-2.6.16-rc2/include/linux/pci.h
> ===================================================================
> --- linux-2.6.16-rc2.orig/include/linux/pci.h 2006-02-14 17:09:24.000000000 +0200
> +++ linux-2.6.16-rc2/include/linux/pci.h 2006-02-14 17:58:17.000000000 +0200
> @@ -95,6 +95,11 @@ enum pci_channel_state {
> pci_channel_io_perm_failure = (__force pci_channel_state_t) 3,
> };
>
> +typedef unsigned short __bitwise pci_bus_flags_t;
> +enum pci_bus_flags {
> + PCI_BUS_FLAGS_NO_MSI = (pci_bus_flags_t) 1,
> +};
> +
> /*
> * The pci_dev structure is used to describe PCI devices.
> */
> @@ -203,7 +208,7 @@ struct pci_bus {
> char name[48];
>
> unsigned short bridge_ctl; /* manage NO_ISA/FBB/et al behaviors */
> - unsigned short pad2;
> + pci_bus_flags_t bus_flags; /* Inherited by child busses */
> struct device *bridge;
> struct class_device class_dev;
> struct bin_attribute *legacy_io; /* legacy I/O for this bus */
>
>
>
Michael, now I'm not sure whether this will work for devices like the
Mellanox PCI-X HCA, where the HCA device sits below a virtual PCI
bridge. In that case we need to propagate the NO_MSI flag from the
8131 bridge to the Tavor bridge, right? And it has to work for
systems like Sun V40Z where the PCI-X slots are hot-swappable (so the
HCA and its bridge could be added later).
- R.
On Tue, Feb 14, 2006 at 10:27:52AM -0800, Roland Dreier wrote:
> Michael, now I'm not sure whether this will work for devices like the
> Mellanox PCI-X HCA, where the HCA device sits below a virtual PCI
> bridge. In that case we need to propagate the NO_MSI flag from the
> 8131 bridge to the Tavor bridge, right? And it has to work for
> systems like Sun V40Z where the PCI-X slots are hot-swappable (so the
> HCA and its bridge could be added later).
Michael's patch does this:
@@ -347,6 +347,7 @@ pci_alloc_child_bus(struct pci_bus *pare
child->parent = parent;
child->ops = parent->ops;
child->sysdata = parent->sysdata;
+ child->bus_flags = parent->bus_flags;
child->bridge = get_device(&bridge->dev);
child->class_dev.class = &pcibus_class;
Quoting r. Roland Dreier <[email protected]>:
> Subject: Re: AMD 8131 and MSI quirk
>
> Michael, now I'm not sure whether this will work for devices like the
> Mellanox PCI-X HCA, where the HCA device sits below a virtual PCI
> bridge. In that case we need to propagate the NO_MSI flag from the
> 8131 bridge to the Tavor bridge, right?
Yes, I tested on that system.
> And it has to work for
> systems like Sun V40Z where the PCI-X slots are hot-swappable (so the
> HCA and its bridge could be added later).
I expect it will work on hot swappable systems too: bus_flags are inherited
from parent to child bus.
--
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies
Quoting Kristen Accardi <[email protected]>:
> > It turns out AMD 8131 quirk only affects MSI for devices behind the 8131
> > bridge. Handle this by adding a flags field in pci_bus, inherited from
> > parent to child.
>
> It seems like we have a way to turn of msi already (the no_msi bit in
> the pci_dev structure). Does it make sense to just have the child bus
> pci_dev structure inherit the no_msi bit from the parent's pci_dev
> structure when doing an allocation, or does that unnecessarily remove
> the msi capability for devices that may not need it?
>
> Kristen
This bit is already used to mean that msi is disabled for a specific device,
which appears to be a PCI Express to PCI bridge (PCI_DEVICE_ID_INTEL_PXH). So
it seems that disabling MSI for child devices as well might break things (i.e.
disable msi unnecessarily).
Working for Intel, I guess you would know about the PCI_DEVICE_ID_INTEL_PXH
best: what do you say?
In my opinion, it is cleaner to separate the two concepts: suppress msi
for child devices versus suppress it for the specific device.
Right?
--
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies
> Michael's patch does this:
>
> @@ -347,6 +347,7 @@ pci_alloc_child_bus(struct pci_bus *pare
> child->parent = parent;
> child->ops = parent->ops;
> child->sysdata = parent->sysdata;
> + child->bus_flags = parent->bus_flags;
> child->bridge = get_device(&bridge->dev);
>
> child->class_dev.class = &pcibus_class;
Sorry, I missed that. Yes, that should work.
- R.
Michael> Yes, I tested on that system.
OK, sorry I missed the bits that made that work.
- R.
On Tue, 2006-02-14 at 23:21 +0200, Michael S. Tsirkin wrote:
> Quoting Kristen Accardi <[email protected]>:
> > > It turns out AMD 8131 quirk only affects MSI for devices behind the 8131
> > > bridge. Handle this by adding a flags field in pci_bus, inherited from
> > > parent to child.
> >
> > It seems like we have a way to turn of msi already (the no_msi bit in
> > the pci_dev structure). Does it make sense to just have the child bus
> > pci_dev structure inherit the no_msi bit from the parent's pci_dev
> > structure when doing an allocation, or does that unnecessarily remove
> > the msi capability for devices that may not need it?
> >
> > Kristen
>
> This bit is already used to mean that msi is disabled for a specific device,
> which appears to be a PCI Express to PCI bridge (PCI_DEVICE_ID_INTEL_PXH). So
> it seems that disabling MSI for child devices as well might break things (i.e.
> disable msi unnecessarily).
> Working for Intel, I guess you would know about the PCI_DEVICE_ID_INTEL_PXH
> best: what do you say?
>
> In my opinion, it is cleaner to separate the two concepts: suppress msi
> for child devices versus suppress it for the specific device.
>
> Right?
>
I was thinking something along these lines might work for you. I think
it does the same thing as the other patch, without needing to add extra
flags to pci_bus. I guess the assumption I made was that if msi is
turned off for a bridge, then all devices under the bridge may not use
msi.
---
drivers/pci/msi.c | 2 +-
drivers/pci/probe.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
--- linux-dock-mm.orig/drivers/pci/msi.c
+++ linux-dock-mm/drivers/pci/msi.c
@@ -701,7 +701,7 @@ int pci_enable_msi(struct pci_dev* dev)
if (!pci_msi_enable || !dev)
return status;
- if (dev->no_msi)
+ if (dev->no_msi || dev->bus->self->no_msi)
return status;
temp = dev->irq;
--- linux-dock-mm.orig/drivers/pci/probe.c
+++ linux-dock-mm/drivers/pci/probe.c
@@ -344,6 +344,7 @@ pci_alloc_child_bus(struct pci_bus *pare
return NULL;
child->self = bridge;
+ child->self->no_msi = parent->self->no_msi;
child->parent = parent;
child->ops = parent->ops;
child->sysdata = parent->sysdata;
Quoting r. Kristen Accardi <[email protected]>:
> I guess the assumption I made was that if msi is
> turned off for a bridge, then all devices under the bridge may not use
> msi.
Well, all this is just quirks, so no real rules apply, thats why I thought
having 2 bits gives us maximum flexibility.
Specifically for PCXH I see this in code:
/*
* It's possible for the MSI to get corrupted if shpc and acpi
* are used together on certain PXH-based systems.
* */
So it seems the issue is device-specific - only affects the bridge itself.
What the code currently does is disable msi for bridge itself but not for
the devices behind it. I cant inherit dev->no_msi from parent to child
without changing this, and I just assumed this is by design.
Are you saying this is a bug and should be changed?
--
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies
On Wed, 2006-02-15 at 00:10 +0200, Michael S. Tsirkin wrote:
> Quoting r. Kristen Accardi <[email protected]>:
> > I guess the assumption I made was that if msi is
> > turned off for a bridge, then all devices under the bridge may not use
> > msi.
>
> Well, all this is just quirks, so no real rules apply, thats why I thought
> having 2 bits gives us maximum flexibility.
>
> Specifically for PCXH I see this in code:
> /*
> * It's possible for the MSI to get corrupted if shpc and acpi
> * are used together on certain PXH-based systems.
> * */
>
> So it seems the issue is device-specific - only affects the bridge itself.
>
> What the code currently does is disable msi for bridge itself but not for
> the devices behind it. I cant inherit dev->no_msi from parent to child
> without changing this, and I just assumed this is by design.
>
> Are you saying this is a bug and should be changed?
>
Ok, I went back and read the errata that this patch was attempting to
solve, and you are right, it would not be correct to have children
inherit the no_msi flag from the parent in this case. I can clearly see
why we need to have a flag associated with the bus rather than the
device for your case.
Kristen
On Tue, Feb 14, 2006 at 06:52:22PM +0200, Michael S. Tsirkin wrote:
> Quoting r. Roland Dreier <[email protected]>:
> > Subject: AMD 8131 and MSI quirk
> >
> > The current quirk_amd_8131_ioapic() function sets a global
> > pci_msi_quirk flag, which disables MSI/MSI-X for all devices in the
> > system. This is safe but suboptimal, because there may be devices on
> > other buses not related to the AMD 8131 bridge, for which MSI would
> > work fine. As an example, see the end of this email for a lspci -t
> > from a real Opteron system that has PCI-X buses coming from an AMD
> > 8131 and PCI Express buses coming from an Nforce4 bridge -- MSI works
> > fine for the Mellanox InfiniBand adapter on the PCIe bus, if we allow
> > it to be enabled.
>
> The following should do this IMO. Roland, could you test this patch please?
>
> ---
>
> It turns out AMD 8131 quirk only affects MSI for devices behind the 8131 bridge.
> Handle this by adding a flags field in pci_bus, inherited from parent to child.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
Roland, did you ever verify that this patch worked or not for you?
thanks,
greg k-h
Greg> Roland, did you ever verify that this patch worked or not for you?
Unfortunately as I said I don't have such a system (with AMD 8131 and
also a host bus controller that _does_ support MSI) myself.
Michael has tested on systems with AMD 8131, so I don't think the
patch could hurt.
- R.