On Thu, 2013-04-11 at 11:23 -0600, Bjorn Helgaas wrote: > On Wed, Apr 10, 2013 at 6:01 PM, Alex Williamson > <alex.williamson@xxxxxxxxxx> wrote: > > On Wed, 2013-04-10 at 16:36 -0600, Bjorn Helgaas wrote: > >> On Wed, Feb 06, 2013 at 08:58:41AM -0700, Alex Williamson wrote: > >> > On Wed, 2013-02-06 at 07:49 -0800, Stephen Hemminger wrote: > >> > > On Mon, 04 Feb 2013 15:41:24 -0700 > >> > > Alex Williamson <alex.williamson@xxxxxxxxxx> wrote: > >> > > > >> > > > On Mon, 2013-02-04 at 13:28 -0700, Alex Williamson wrote: > >> > > > > On Mon, 2013-02-04 at 10:36 -0800, Stephen Hemminger wrote: > >> > > > > > > I think drivers/pci/search.c is identical between 3.7 and 3.8-rc1. Is > >> > > > > > > this the first time you've turned on the IOMMU on that box? > >> > > > > > > >> > > > > > It exists in 3.7 and earlier kernels, just haven't turned on same config. > >> > > > > > > >> > > > > > > It's the same warning as in this bugzilla: > >> > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=44881, and there's a patch > >> > > > > > > there at https://bugzilla.kernel.org/show_bug.cgi?id=44881#c11, but > >> > > > > > > it's just a quirk that turns off VT-d if we find certain broken > >> > > > > > > bridges. It doesn't look like you have any of those (although I don't > >> > > > > > > know what you have at 05:00.0). > >> > > > > > > > >> > > > > > > Bjorn > >> > > > > > > >> > > > > > This is a standard ASUS motherboard, and don't want to disable VT-d. > >> > > > > > >> > > > > Stephen, > >> > > > > > >> > > > > Can you give the lspci -vvv of device 5:00.0 to see if it's one we've > >> > > > > seen before? Does the patch below help? > >> > > > > > >> > > > > Bjorn, I think we need to quirk it somehow. So far they've all been > >> > > > > PCI-to-PCI bridges attached to root ports where we expect it's actually > >> > > > > a PCIe-to-PCI bridge. Seems like maybe we could have the same attached > >> > > > > to a downstream port. The patch below avoids the WARN and gives us a > >> > > > > device, but of course pci_is_pcie reports wrong for this device and may > >> > > > > cause some trickle down breakage. A more complete option might be to > >> > > > > add a is_pcie flag to the device that can be set independent of > >> > > > > pcie_cap. We'd need to check all the callers for assumptions, but then > >> > > > > we could put the quirk in one place and hopefully fix everything. > >> > > > > Thoughts? Thanks, > >> > > > > >> > > > This latter approach seems like it might be easier than I expected since > >> > > > all the users are so well filtered through the access functions. A > >> > > > quick look through who uses pci_is_pcie seems like this might be > >> > > > complete, but more eyes are required. I'll upload this to the bz for > >> > > > those reporters to test as well. Thoughts? Thanks, > >> > > > > >> > > > Alex > >> > > > >> > > On my hardware this gives: > >> > > >> > > [ 0.254621] pci_bus 0000:05: busn_res: can not insert [bus 05-ff] under [bus 00-3e] (conflicts with (null) [bus 00-3e]) > >> > > [ 0.254647] WARNING: Your hardware is broken, device (null) appears to be a > >> > > [ 0.254647] Legacy PCI device attached directly to a PCIe device which is not a > >> > > [ 0.254647] PCIe-to-PCI bridge. Per section 7.8 of the PCI Express 3.0 spec, the > >> > > [ 0.254647] PCI express capability structure is required for PCI express device > >> > > [ 0.254647] functions. > >> > > [ 0.254653] pci 0000:05:00.0: [1b21:1080] type 01 class 0x060401 > >> > > >> > I guess I must be calling pci_name() before it's set. The warning > >> > message needs some work too, it's mainly meant for hardware vendors with > >> > the hope that they might test Linux and see it before shipping these > >> > broken devices. Bjorn, does this approach seem worth pursuing? Thanks, > >> > >> Sorry I dropped this for so long. I'm looking at the patch > >> here: https://bugzilla.kernel.org/attachment.cgi?id=92521, > >> appended for convenience. > >> > >> In case anybody else needs the context, I think we have > >> this scenario (from John Wehin's original report at > >> https://bugzilla.kernel.org/show_bug.cgi?id=44881): > >> > >> pci 0000:00:1c.4: PCI bridge to [bus 03-04] # PCIe root port > >> pci 0000:03:00.0: PCI bridge to [bus 04] # no PCIe cap > >> ... > >> pci 0000:03:00.0: expected upstream PCIe bridge; 0000:00:1c.4 is type 0x4 > >> > >> We called pci_find_upstream_pcie_bridge(03:00.0), which generated > >> the warning because: > >> > >> - 03:00.0 is not a PCIe device, and > >> - 00:1c.4 (its upstream bridge) *is* a PCIe device, and > >> - 00:1c.4 is a Root Port (PCI_EXP_TYPE_ROOT_PORT == 0x4), > >> not a PCIe-to-PCI bridge (PCI_EXP_TYPE_PCI_BRIDGE == 0x7) > >> as we expected > >> > >> > commit 60d668a3cdeeb0e29570cf0043736436c146bde8 > >> > Author: Alex Williamson <alex.williamson@xxxxxxxxxx> > >> > Date: Mon Feb 4 15:34:34 2013 -0700 > >> > > >> > pci: Handle unadvertised PCIe bridges > >> > > >> > There seem to be several PCIe-to-PCI bridges out in the wild that > >> > blatantly ignore the PCIe specification and do not expose a PCIe > >> > capability. We can attempt to deduce their existence by looking > >> > for PCI bridges directly connected to root ports or downstream > >> > ports. What this means is that pci_is_pcie() does not imply PCIe > >> > capability and we un-deprecate is_pcie to denote the difference. > >> > All the accesses seem to go through pcie_capability_reg_implemented, > >> > so we can significantly limit the footprint of this change by > >> > checking things there. > >> > > >> > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx> > >> > > >> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c > >> > index 3af0478..3df24e7 100644 > >> > --- a/drivers/pci/access.c > >> > +++ b/drivers/pci/access.c > >> > @@ -511,7 +511,7 @@ static inline bool pcie_cap_has_rtctl(const struct pci_dev *dev) > >> > > >> > static bool pcie_capability_reg_implemented(struct pci_dev *dev, int pos) > >> > { > >> > - if (!pci_is_pcie(dev)) > >> > + if (!pci_is_pcie(dev) || !pci_pcie_cap(dev)) > >> > return false; > >> > > >> > switch (pos) { > >> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > >> > index 6186f03..0a87b6b 100644 > >> > --- a/drivers/pci/probe.c > >> > +++ b/drivers/pci/probe.c > >> > @@ -926,20 +926,46 @@ static void pci_read_irq(struct pci_dev *dev) > >> > dev->irq = irq; > >> > } > >> > > >> > +static bool is_unadvertised_pcie_bridge(struct pci_dev *pdev) > >> > +{ > >> > + struct pci_dev *parent; > >> > + > >> > + if (pdev->hdr_type != PCI_HEADER_TYPE_BRIDGE || > >> > + pci_find_capability(pdev, PCI_CAP_ID_EXP) || > >> > + pci_is_root_bus(pdev->bus)) > >> > + return false; > >> > + > >> > + parent = pdev->bus->self; > >> > + > >> > + if (pci_is_pcie(parent) && > >> > + (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT || > >> > + pci_pcie_type(parent) == PCI_EXP_TYPE_DOWNSTREAM)) { > >> > + pr_warn("WARNING: Your hardware is broken, device %s appears to be a\n Legacy PCI device attached directly to a PCIe device which is not a\n PCIe-to-PCI bridge. Per section 7.8 of the PCI Express 3.0 spec, the\n PCI express capability structure is required for PCI express device\nfunctions.\n", > >> > + pci_name(pdev)); > >> > >> Vendors might see this warning, but I'm doubtful they'll do anything > >> about it. I suspect it will result in a lot of emails from concerned > >> users to LKML and linux-pci, and we really can't do anything other > >> than say "yup, it's broken, report it to your vendor." > >> > >> And since the hardware seems to actually *work* if we just pretend that > >> the problem device (e.g., 03:00.0 above) is PCIe, it's doubtful that > >> the vendor would do anything anyway, so maybe a dev_info() would be > >> sufficient. > >> > >> > + return true; > >> > + } > >> > + > >> > + return false; > >> > +} > >> > + > >> > void set_pcie_port_type(struct pci_dev *pdev) > >> > { > >> > int pos; > >> > - u16 reg16; > >> > + u16 flags, caps = 0; > >> > > >> > pos = pci_find_capability(pdev, PCI_CAP_ID_EXP); > >> > - if (!pos) > >> > + if (pos) { > >> > + pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &flags); > >> > + pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &caps); > >> > + } else if (is_unadvertised_pcie_bridge(pdev)) > >> > + flags = PCI_EXP_TYPE_PCI_BRIDGE << 4; > >> > + else > >> > return; > >> > + > >> > pdev->is_pcie = 1; > >> > pdev->pcie_cap = pos; > >> > - pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, ®16); > >> > - pdev->pcie_flags_reg = reg16; > >> > - pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, ®16); > >> > - pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD; > >> > + pdev->pcie_flags_reg = flags; > >> > >> If we can avoid it, I'd prefer not to complicate the meaning of > >> "pci_is_pcie()" -- it used to mean "this device has a PCIe > >> capability and you can do PCIe things with it." But now it > >> means something else, and we can't do PCIe things with these > >> problem devices anyway. > >> > >> Could we accomplish basically the same thing by making > >> pci_find_upstream_pcie_bridge() look like this? > >> > >> if (pci_is_pcie(pdev)) > >> return NULL; > >> > >> + bridge = pdev->bus->self; > >> + if (bridge && pci_is_pcie(bridge) && > >> + (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT || > >> + pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM)) > >> + return NULL; > >> > >> while (1) { > >> ... > > > > This only solves pci_find_upstream_pcie_bridge(3:00.0), I think it still > > fails for any devices found on subordinate buses below that. Thanks, > > Can't we apply the same approach throughout the whole tree with some > reworking of pci_find_upstream_pcie_bridge()? > > It seems like pci_find_upstream_pcie_bridge() (and some code in > callers) is really trying to figure out the requester-ID for use as > the IOMMU's source-ID, but the current code organization seems a bit > confusing. I suspect cleaning that up a bit would make it more > obvious how to fix this. But the bug is really that the bridge is a PCIe device but does not expose a PCIe capability. So yes, we can add hacks all around this path to fix it, but we lose the general ability to identify these devices as PCIe. Maybe a compromise is a version of pci_is_pcie() that is a little more flexible, pci_is_probably_pcie()? Then we could use it when we don't actually want to access the capability, but want to test the device type. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
