Re: PCI warning on boot 3.8.0-rc1

Re: PCI warning on boot 3.8.0-rc1

[Bjorn Helgaas]

On Wed, Jan 16, 2013 at 3:38 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> I see this on boot in dmesg
>
> [    0.574494] DMAR: No ATSR found
> [    0.574549] IOMMU 0 0xfed90000: using Queued invalidation
> [    0.574550] IOMMU 1 0xfed91000: using Queued invalidation
> [    0.574554] IOMMU: Setting RMRR:
> [    0.574583] IOMMU: Setting identity map for device 0000:00:02.0 [0xcf000000 -
>  0xdf1fffff]
> [    0.575748] IOMMU: Setting identity map for device 0000:00:1d.0 [0xcd551000 -
>  0xcd56dfff]
> [    0.575767] IOMMU: Setting identity map for device 0000:00:1a.0 [0xcd551000 -
>  0xcd56dfff]
> [    0.575786] IOMMU: Setting identity map for device 0000:00:14.0 [0xcd551000 -
>  0xcd56dfff]
> [    0.575797] IOMMU: Prepare 0-16MiB unity mapping for LPC
> [    0.575806] IOMMU: Setting identity map for device 0000:00:1f.0 [0x0 - 0xffff
> ff]
> [    0.576186] PCI-DMA: Intel(R) Virtualization Technology for Directed I/O
> [    0.576337] ------------[ cut here ]------------
> [    0.576342] WARNING: at drivers/pci/search.c:46 pci_find_upstream_pcie_bridge
> +0x59/0x71()
> [    0.576343] Hardware name: System Product Name
> [    0.576344] Modules linked in:
> [    0.576347] Pid: 1, comm: swapper/0 Not tainted 3.8.0-rc1-net-next+ #4
> [    0.576348] Call Trace:
> [    0.576352]  [<ffffffff81074261>] warn_slowpath_common+0x83/0x9c
> [    0.576355]  [<ffffffff813fae71>] ? bus_set_iommu+0x49/0x49
> [    0.576357]  [<ffffffff81074294>] warn_slowpath_null+0x1a/0x1c
> [    0.576360]  [<ffffffff812db618>] pci_find_upstream_pcie_bridge+0x59/0x71
> [    0.576362]  [<ffffffff813fd247>] intel_iommu_add_device+0x4d/0x17a
> [    0.576364]  [<ffffffff813faeab>] add_iommu_group+0x3a/0x48
> [    0.576368]  [<ffffffff8135d490>] bus_for_each_dev+0x57/0x89
> [    0.576370]  [<ffffffff813fae6a>] bus_set_iommu+0x42/0x49
> [    0.576374]  [<ffffffff81b1b1a5>] intel_iommu_init+0xa27/0xb44
> [    0.576377]  [<ffffffff8105dcf3>] ? free_init_pages+0xf5/0x10d
> [    0.576379]  [<ffffffff81aeb87d>] ? maybe_link.part.2+0x10b/0x10b
> [    0.576382]  [<ffffffff81aeef53>] ? memblock_find_dma_reserve+0x133/0x133
> [    0.576384]  [<ffffffff81aeef66>] pci_iommu_init+0x13/0x3e
> [    0.576387]  [<ffffffff8100023c>] do_one_initcall+0x7f/0x133
> [    0.576390]  [<ffffffff814d0c2c>] kernel_init+0x146/0x29b
> [    0.576393]  [<ffffffff81aea52f>] ? do_early_param+0x8c/0x8c
> [    0.576395]  [<ffffffff814d0ae6>] ? rest_init+0xda/0xda
> [    0.576398]  [<ffffffff814ed4dc>] ret_from_fork+0x7c/0xb0
> [    0.576400]  [<ffffffff814d0ae6>] ? rest_init+0xda/0xda
> [    0.576406] ---[ end trace f709d1eb2b66cbf5 ]---
>
> $ sudo lspci -t -vv
> -[0000:00]-+-00.0  Intel Corporation Xeon E3-1200 v2/3rd Gen Core processor DRAM Controller
>            +-01.0-[01-02]----00.0  Intel Corporation Ethernet Controller 10-Gigabit X540-AT2
>            +-02.0  Intel Corporation Xeon E3-1200 v2/3rd Gen Core processor Graphics Controller
>            +-14.0  Intel Corporation 7 Series/C210 Series Chipset Family USB xHCI Host Controller
>            +-16.0  Intel Corporation 7 Series/C210 Series Chipset Family MEI Controller #1
>            +-1a.0  Intel Corporation 7 Series/C210 Series Chipset Family USB Enhanced Host Controller #2
>            +-1b.0  Intel Corporation 7 Series/C210 Series Chipset Family High Definition Audio Controller
>            +-1c.0-[03]--
>            +-1c.4-[04]----00.0  Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller
>            +-1c.5-[05-06]----00.0-[06]--
>            +-1d.0  Intel Corporation 7 Series/C210 Series Chipset Family USB Enhanced Host Controller #1
>            +-1f.0  Intel Corporation Z77 Express Chipset LPC Controller
>            +-1f.2  Intel Corporation 7 Series/C210 Series Chipset Family 6-port SATA Controller [AHCI mode]
>            \-1f.3  Intel Corporation 7 Series/C210 Series Chipset Family SMBus Controller

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'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

Re: PCI warning on boot 3.8.0-rc1

[Stephen Hemminger]

> 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.

Re: PCI warning on boot 3.8.0-rc1

[Alex Williamson]

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,

Alex


diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index bf969ba..65ae270 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -42,6 +42,15 @@ pci_find_upstream_pcie_bridge(struct pci_dev *pdev)
 		}
 		/* PCI device should connect to a PCIe bridge */
 		if (pci_pcie_type(pdev) != PCI_EXP_TYPE_PCI_BRIDGE) {
+			/*
+			 * Not all PCIe-to-PCI bridges expose a PCIe
+			 * capability.  If we make it to a PCIe root port
+			 * and the previous device was a PCI-to-PCI bridge,
+			 * assume it was really a PCIe-to-PCI bridge.
+			 */
+			if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
+			    tmp && tmp->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+				return tmp;
 			/* Busted hardware? */
 			WARN_ON_ONCE(1);
 			return NULL;

Re: PCI warning on boot 3.8.0-rc1

[Stephen Hemminger]

On Mon, 04 Feb 2013 13:28:04 -0700
Alex Williamson <alex.williamson@redhat.com> 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?


$ sudo lspci -vvv -s 5:00.0
05:00.0 PCI bridge: ASMedia Technology Inc. ASM1083/1085 PCIe to PCI Bridge (rev 03) (prog-if 01 [Subtractive decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Bus: primary=05, secondary=06, subordinate=06, sec-latency=32
	I/O behind bridge: 0000f000-00000fff
	Memory behind bridge: fff00000-000fffff
	Prefetchable memory behind bridge: 00000000fff00000-00000000000fffff
	Secondary status: 66MHz+ FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ <SERR- <PERR-
	BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [c0] Subsystem: ASUSTeK Computer Inc. Device 8489


> 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,
> 
> Alex
> 
> 
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index bf969ba..65ae270 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -42,6 +42,15 @@ pci_find_upstream_pcie_bridge(struct pci_dev *pdev)
>  		}
>  		/* PCI device should connect to a PCIe bridge */
>  		if (pci_pcie_type(pdev) != PCI_EXP_TYPE_PCI_BRIDGE) {
> +			/*
> +			 * Not all PCIe-to-PCI bridges expose a PCIe
> +			 * capability.  If we make it to a PCIe root port
> +			 * and the previous device was a PCI-to-PCI bridge,
> +			 * assume it was really a PCIe-to-PCI bridge.
> +			 */
> +			if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
> +			    tmp && tmp->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> +				return tmp;
>  			/* Busted hardware? */
>  			WARN_ON_ONCE(1);
>  			return NULL;
> 
> 

Re: PCI warning on boot 3.8.0-rc1

[Alex Williamson]

On Mon, 2013-02-04 at 12:30 -0800, Stephen Hemminger wrote:
> On Mon, 04 Feb 2013 13:28:04 -0700
> Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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?
> 
> 
> $ sudo lspci -vvv -s 5:00.0
> 05:00.0 PCI bridge: ASMedia Technology Inc. ASM1083/1085 PCIe to PCI Bridge (rev 03) (prog-if 01 [Subtractive decode])

Someone has even decided to name this one a PCIe-to-PCI bridge despite
it lacking a PCIe capability.  Thanks,

Alex

> 	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
> 	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
> 	Latency: 0, Cache Line Size: 64 bytes
> 	Bus: primary=05, secondary=06, subordinate=06, sec-latency=32
> 	I/O behind bridge: 0000f000-00000fff
> 	Memory behind bridge: fff00000-000fffff
> 	Prefetchable memory behind bridge: 00000000fff00000-00000000000fffff
> 	Secondary status: 66MHz+ FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ <SERR- <PERR-
> 	BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
> 		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
> 	Capabilities: [c0] Subsystem: ASUSTeK Computer Inc. Device 8489
> 
> 
> > 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,
> > 
> > Alex
> > 
> > 
> > diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> > index bf969ba..65ae270 100644
> > --- a/drivers/pci/search.c
> > +++ b/drivers/pci/search.c
> > @@ -42,6 +42,15 @@ pci_find_upstream_pcie_bridge(struct pci_dev *pdev)
> >  		}
> >  		/* PCI device should connect to a PCIe bridge */
> >  		if (pci_pcie_type(pdev) != PCI_EXP_TYPE_PCI_BRIDGE) {
> > +			/*
> > +			 * Not all PCIe-to-PCI bridges expose a PCIe
> > +			 * capability.  If we make it to a PCIe root port
> > +			 * and the previous device was a PCI-to-PCI bridge,
> > +			 * assume it was really a PCIe-to-PCI bridge.
> > +			 */
> > +			if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
> > +			    tmp && tmp->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> > +				return tmp;
> >  			/* Busted hardware? */
> >  			WARN_ON_ONCE(1);
> >  			return NULL;
> > 
> > 
> 

Re: PCI warning on boot 3.8.0-rc1

[Alex Williamson]

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

commit 60d668a3cdeeb0e29570cf0043736436c146bde8
Author: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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));
+		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, &reg16);
-	pdev->pcie_flags_reg = reg16;
-	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
-	pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
+	pdev->pcie_flags_reg = flags;
+	pdev->pcie_mpss = caps & PCI_EXP_DEVCAP_PAYLOAD;
 }
 
 void set_pcie_hotplug_bridge(struct pci_dev *pdev)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 15472d6..c9ef42c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -300,8 +300,8 @@ struct pci_dev {
 	unsigned int	msix_enabled:1;
 	unsigned int	ari_enabled:1;	/* ARI forwarding */
 	unsigned int	is_managed:1;
-	unsigned int	is_pcie:1;	/* Obsolete. Will be removed.
-					   Use pci_is_pcie() instead */
+	unsigned int	is_pcie:1;	/* Don't access directly,
+					   use pci_is_pcie() instead */
 	unsigned int    needs_freset:1; /* Dev requires fundamental reset */
 	unsigned int	state_saved:1;
 	unsigned int	is_physfn:1;
@@ -1689,7 +1689,7 @@ static inline int pci_pcie_cap(struct pci_dev *dev)
  */
 static inline bool pci_is_pcie(struct pci_dev *dev)
 {
-	return !!pci_pcie_cap(dev);
+	return dev->is_pcie;
 }
 
 /**

Re: PCI warning on boot 3.8.0-rc1

[Stephen Hemminger]

On Mon, 04 Feb 2013 15:41:24 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> +
> +	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));

Too whiny!
Two short lines seems to be more the enough. Users can't do anything about
hardware not following spec. Put any outher stuff into comments.

Re: PCI warning on boot 3.8.0-rc1

[Stephen Hemminger]

On Mon, 04 Feb 2013 15:41:24 -0700
Alex Williamson <alex.williamson@redhat.com> 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.000000] Initializing cgroup subsys cpuset
[    0.000000] Initializing cgroup subsys cpu
[    0.000000] Linux version 3.8.0-rc6+ (shemminger@nehalam) (gcc version 4.7.2 (Debian 4.7.2-5) ) #3 SMP Tue Feb 5 20:34:31 PST 2013
[    0.000000] Command line: BOOT_IMAGE=/boot/vmlinuz-3.8.0-rc6+ root=UUID=1772a2c4-c197-4b0a-84dd-9982243242b7 ro quiet pci=assign-busses
...
[    0.226369] ACPI: No dock devices found.
[    0.226373] PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
[    0.226734] ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-3e])
[    0.226737] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0._PRT]
[    0.227337] pci_root PNP0A08:00: Requesting ACPI _OSC control (0x1d)
[    0.227688] pci_root PNP0A08:00: ACPI _OSC control (0x18) granted
[    0.229268] PCI host bridge to bus 0000:00
[    0.229271] pci_bus 0000:00: root bus resource [bus 00-3e]
[    0.229273] pci_bus 0000:00: root bus resource [io  0x0000-0x0cf7]
[    0.229275] pci_bus 0000:00: root bus resource [io  0x0d00-0xffff]
[    0.229276] pci_bus 0000:00: root bus resource [mem 0x000a0000-0x000bffff]
[    0.229278] pci_bus 0000:00: root bus resource [mem 0x000d0000-0x000d3fff]
[    0.229280] pci_bus 0000:00: root bus resource [mem 0x000d4000-0x000d7fff]
[    0.229281] pci_bus 0000:00: root bus resource [mem 0x000d8000-0x000dbfff]
[    0.229283] pci_bus 0000:00: root bus resource [mem 0x000dc000-0x000dffff]
[    0.229286] pci_bus 0000:00: root bus resource [mem 0x000e0000-0x000e3fff]
[    0.229287] pci_bus 0000:00: root bus resource [mem 0x000e4000-0x000e7fff]
[    0.229289] pci_bus 0000:00: root bus resource [mem 0xdf200000-0xfeafffff]
[    0.229316] pci 0000:00:00.0: [8086:0150] type 00 class 0x060000
[    0.229386] pci 0000:00:01.0: [8086:0151] type 01 class 0x060400
[    0.229427] pci 0000:00:01.0: PME# supported from D0 D3hot D3cold
[    0.229470] pci 0000:00:02.0: [8086:0162] type 00 class 0x030000
[    0.229482] pci 0000:00:02.0: reg 10: [mem 0xf7800000-0xf7bfffff 64bit]
[    0.229490] pci 0000:00:02.0: reg 18: [mem 0xe0000000-0xefffffff 64bit pref]
[    0.229495] pci 0000:00:02.0: reg 20: [io  0xf000-0xf03f]
[    0.229560] pci 0000:00:14.0: [8086:1e31] type 00 class 0x0c0330
[    0.229584] pci 0000:00:14.0: reg 10: [mem 0xf7d00000-0xf7d0ffff 64bit]
[    0.229667] pci 0000:00:14.0: PME# supported from D3hot D3cold
[    0.229693] pci 0000:00:16.0: [8086:1e3a] type 00 class 0x078000
[    0.229719] pci 0000:00:16.0: reg 10: [mem 0xf7d1a000-0xf7d1a00f 64bit]
[    0.229805] pci 0000:00:16.0: PME# supported from D0 D3hot D3cold
[    0.229844] pci 0000:00:1a.0: [8086:1e2d] type 00 class 0x0c0320
[    0.229868] pci 0000:00:1a.0: reg 10: [mem 0xf7d18000-0xf7d183ff]
[    0.229972] pci 0000:00:1a.0: PME# supported from D0 D3hot D3cold
[    0.230002] pci 0000:00:1b.0: [8086:1e20] type 00 class 0x040300
[    0.230018] pci 0000:00:1b.0: reg 10: [mem 0xf7d10000-0xf7d13fff 64bit]
[    0.230098] pci 0000:00:1b.0: PME# supported from D0 D3hot D3cold
[    0.230126] pci 0000:00:1c.0: [8086:1e10] type 01 class 0x060400
[    0.230217] pci 0000:00:1c.0: PME# supported from D0 D3hot D3cold
[    0.230250] pci 0000:00:1c.4: [8086:1e18] type 01 class 0x060400
[    0.230340] pci 0000:00:1c.4: PME# supported from D0 D3hot D3cold
[    0.230370] pci 0000:00:1c.5: [8086:244e] type 01 class 0x060401
[    0.230461] pci 0000:00:1c.5: PME# supported from D0 D3hot D3cold
[    0.230498] pci 0000:00:1d.0: [8086:1e26] type 00 class 0x0c0320
[    0.230525] pci 0000:00:1d.0: reg 10: [mem 0xf7d17000-0xf7d173ff]
[    0.230629] pci 0000:00:1d.0: PME# supported from D0 D3hot D3cold
[    0.230659] pci 0000:00:1f.0: [8086:1e44] type 00 class 0x060100
[    0.230799] pci 0000:00:1f.2: [8086:1e02] type 00 class 0x010601
[    0.230820] pci 0000:00:1f.2: reg 10: [io  0xf0b0-0xf0b7]
[    0.230828] pci 0000:00:1f.2: reg 14: [io  0xf0a0-0xf0a3]
[    0.230837] pci 0000:00:1f.2: reg 18: [io  0xf090-0xf097]
[    0.230845] pci 0000:00:1f.2: reg 1c: [io  0xf080-0xf083]
[    0.230853] pci 0000:00:1f.2: reg 20: [io  0xf060-0xf07f]
[    0.230862] pci 0000:00:1f.2: reg 24: [mem 0xf7d16000-0xf7d167ff]
[    0.230915] pci 0000:00:1f.2: PME# supported from D3hot
[    0.230936] pci 0000:00:1f.3: [8086:1e22] type 00 class 0x0c0500
[    0.230952] pci 0000:00:1f.3: reg 10: [mem 0xf7d15000-0xf7d150ff 64bit]
[    0.230976] pci 0000:00:1f.3: reg 20: [io  0xf040-0xf05f]
[    0.231054] pci_bus 0000:01: busn_res: can not insert [bus 01-ff] under [bus 00-3e] (conflicts with (null) [bus 00-3e])
[    0.231071] pci 0000:01:00.0: [8086:1528] type 00 class 0x020000
[    0.231084] pci 0000:01:00.0: reg 10: [mem 0xf0000000-0xf01fffff 64bit pref]
[    0.231108] pci 0000:01:00.0: reg 20: [mem 0xf0200000-0xf0203fff 64bit pref]
[    0.231115] pci 0000:01:00.0: reg 30: [mem 0xf7c00000-0xf7c7ffff pref]
[    0.231154] pci 0000:01:00.0: PME# supported from D0 D3hot
[    0.231185] pci 0000:01:00.0: reg 184: [mem 0x00000000-0x00003fff 64bit]
[    0.231200] pci 0000:01:00.0: reg 190: [mem 0x00000000-0x00003fff 64bit]
[    0.238547] pci 0000:00:01.0: PCI bridge to [bus 01-ff]
[    0.238565] pci 0000:00:01.0:   bridge window [mem 0xf7c00000-0xf7cfffff]
[    0.238568] pci 0000:00:01.0:   bridge window [mem 0xf0000000-0xf02fffff 64bit pref]
[    0.238571] pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 02
[    0.238623] pci_bus 0000:03: busn_res: can not insert [bus 03-ff] under [bus 00-3e] (conflicts with (null) [bus 00-3e])
[    0.238628] pci 0000:00:1c.0: PCI bridge to [bus 03-ff]
[    0.238639] pci_bus 0000:03: busn_res: [bus 03-ff] end is updated to 03
[    0.238690] pci_bus 0000:04: busn_res: can not insert [bus 04-ff] under [bus 00-3e] (conflicts with (null) [bus 00-3e])
[    0.238719] pci 0000:04:00.0: [10ec:8168] type 00 class 0x020000
[    0.238740] pci 0000:04:00.0: reg 10: [io  0xe000-0xe0ff]
[    0.238777] pci 0000:04:00.0: reg 18: [mem 0xf0404000-0xf0404fff 64bit pref]
[    0.238800] pci 0000:04:00.0: reg 20: [mem 0xf0400000-0xf0403fff 64bit pref]
[    0.238902] pci 0000:04:00.0: supports D1 D2
[    0.238903] pci 0000:04:00.0: PME# supported from D0 D1 D2 D3hot D3cold
[    0.254537] pci 0000:00:1c.4: PCI bridge to [bus 04-ff]
[    0.254544] pci 0000:00:1c.4:   bridge window [io  0xe000-0xefff]
[    0.254566] pci 0000:00:1c.4:   bridge window [mem 0xf0400000-0xf04fffff 64bit pref]
[    0.254568] pci_bus 0000:04: busn_res: [bus 04-ff] end is updated to 04
[    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
[    0.266521] pci 0000:00:1c.5: PCI bridge to [bus 05-ff] (subtractive decode)
[    0.266540] pci 0000:00:1c.5:   bridge window [io  0x0000-0x0cf7] (subtractive decode)
[    0.266543] pci 0000:00:1c.5:   bridge window [io  0x0d00-0xffff] (subtractive decode)
[    0.266547] pci 0000:00:1c.5:   bridge window [mem 0x000a0000-0x000bffff] (subtractive decode)
[    0.266550] pci 0000:00:1c.5:   bridge window [mem 0x000d0000-0x000d3fff] (subtractive decode)
[    0.266554] pci 0000:00:1c.5:   bridge window [mem 0x000d4000-0x000d7fff] (subtractive decode)
[    0.266566] pci 0000:00:1c.5:   bridge window [mem 0x000d8000-0x000dbfff] (subtractive decode)
[    0.266567] pci 0000:00:1c.5:   bridge window [mem 0x000dc000-0x000dffff] (subtractive decode)
[    0.266569] pci 0000:00:1c.5:   bridge window [mem 0x000e0000-0x000e3fff] (subtractive decode)
[    0.266570] pci 0000:00:1c.5:   bridge window [mem 0x000e4000-0x000e7fff] (subtractive decode)
[    0.266572] pci 0000:00:1c.5:   bridge window [mem 0xdf200000-0xfeafffff] (subtractive decode)
[    0.266576] pci 0000:05:00.0: bridge configuration invalid ([bus 05-05]), reconfiguring
[    0.266715] pci 0000:05:00.0: PCI bridge to [bus 06-ff] (subtractive decode)
[    0.266739] pci 0000:05:00.0:   bridge window [??? 0x00000000 flags 0x0] (subtractive decode)
[    0.266740] pci 0000:05:00.0:   bridge window [??? 0x00000000 flags 0x0] (subtractive decode)
[    0.266742] pci 0000:05:00.0:   bridge window [??? 0x00000000 flags 0x0] (subtractive decode)
[    0.266744] pci 0000:05:00.0:   bridge window [??? 0x00000000 flags 0x0] (subtractive decode)
[    0.266745] pci 0000:05:00.0:   bridge window [io  0x0000-0x0cf7] (subtractive decode)
[    0.266747] pci 0000:05:00.0:   bridge window [io  0x0d00-0xffff] (subtractive decode)
[    0.266748] pci 0000:05:00.0:   bridge window [mem 0x000a0000-0x000bffff] (subtractive decode)
[    0.266750] pci 0000:05:00.0:   bridge window [mem 0x000d0000-0x000d3fff] (subtractive decode)
[    0.266752] pci 0000:05:00.0:   bridge window [mem 0x000d4000-0x000d7fff] (subtractive decode)
[    0.266753] pci 0000:05:00.0:   bridge window [mem 0x000d8000-0x000dbfff] (subtractive decode)
[    0.266755] pci 0000:05:00.0:   bridge window [mem 0x000dc000-0x000dffff] (subtractive decode)
[    0.266756] pci 0000:05:00.0:   bridge window [mem 0x000e0000-0x000e3fff] (subtractive decode)
[    0.266758] pci 0000:05:00.0:   bridge window [mem 0x000e4000-0x000e7fff] (subtractive decode)
[    0.266760] pci 0000:05:00.0:   bridge window [mem 0xdf200000-0xfeafffff] (subtractive decode)
[    0.266761] pci_bus 0000:06: busn_res: [bus 06-ff] end is updated to 06
[    0.266768] pci_bus 0000:05: busn_res: [bus 05-ff] end is updated to 06
[    0.266814] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.RP01._PRT]
[    0.266887] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.RP05._PRT]
[    0.266946] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.RP06._PRT]
[    0.267008] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.RP06.P9PE._PRT]
[    0.267103] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.PEG0._PRT]
[    0.275012] ACPI: PCI Interrupt Link [LNKA] (IRQs 3 4 5 6 10 *11 12 14 15)
[    0.275086] ACPI: PCI Interrupt Link [LNKB] (IRQs 3 4 5 6 10 *11 12 14 15)
[    0.275157] ACPI: PCI Interrupt Link [LNKC] (IRQs 3 4 5 6 *10 11 12 14 15)
[    0.275227] ACPI: PCI Interrupt Link [LNKD] (IRQs 3 4 *5 6 10 11 12 14 15)
[    0.275298] ACPI: PCI Interrupt Link [LNKE] (IRQs 3 4 5 6 10 11 12 14 15) *0, disabled.
[    0.275369] ACPI: PCI Interrupt Link [LNKF] (IRQs 3 4 5 6 *10 11 12 14 15)
[    0.275444] ACPI: PCI Interrupt Link [LNKG] (IRQs 3 *4 5 6 10 11 12 14 15)
[    0.275514] ACPI: PCI Interrupt Link [LNKH] (IRQs *3 4 5 6 10 11 12 14 15)
[    0.275748] vgaarb: device added: PCI:0000:00:02.0,decodes=io+mem,owns=io+mem,locks=none
[    0.275754] vgaarb: loaded
[    0.275755] vgaarb: bridge control possible 0000:00:02.0
[    0.275888] SCSI subsystem initialized
[    0.275890] ACPI: bus type scsi registered
[    0.275938] libata version 3.00 loaded.
[    0.275947] ACPI: bus type usb registered

Re: PCI warning on boot 3.8.0-rc1

[Alex Williamson]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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,

Alex

Re: PCI warning on boot 3.8.0-rc1

[Alex Williamson]

On Wed, 2013-02-06 at 08:58 -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@redhat.com> 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,

I don't know if it sways how we handle this devices, but a couple notes
on the asmedia chip.  I have one in a non-VT-d capable system and an
add-in legacy PCI NIC shows up behind it when added to the system.  The
chip is visible on the board and is an ASM1083.  Asmedia's website of
course claims this device is fully compliant with the PCIe-to-PCI bridge
spec, ignoring the multiple statements the spec contains requiring such
devices to support a PCIe capability.

Additionally, if you google for ASM1083 you'll find the next highest
links after the product links are bug reports that not only is this
device non-spec complaint, but it doesn't work.  There seems to be an
issue with how INTx is de-asserted (or not) leading to interrupt storms
and requiring the use of irqpoll.  Sure enough, the tulip card I
installed generated some of these and is operating in polling mode.  The
threads indicate that these issues are not isolated to Linux and Windows
users also complain about devices not working or having poor performance
installed behind this bridge.  All in all, it's an absurdly broken piece
of hardware.

I wonder if instead of trying to work around it, we should just
blacklist the device and ignore that it even exists.  Stop the bus walk
with some kind of dmesg error and provide a boot time option to scan it.
It's not the most user friendly option, but a) most people don't seem to
have anything behind it, b) it barely works if they do.  Thanks,

Alex

Re: PCI warning on boot 3.8.0-rc1

[Stephen Hemminger]

On Mon, 11 Feb 2013 21:15:56 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Wed, 2013-02-06 at 08:58 -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@redhat.com> 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,
> 
> I don't know if it sways how we handle this devices, but a couple notes
> on the asmedia chip.  I have one in a non-VT-d capable system and an
> add-in legacy PCI NIC shows up behind it when added to the system.  The
> chip is visible on the board and is an ASM1083.  Asmedia's website of
> course claims this device is fully compliant with the PCIe-to-PCI bridge
> spec, ignoring the multiple statements the spec contains requiring such
> devices to support a PCIe capability.
> 
> Additionally, if you google for ASM1083 you'll find the next highest
> links after the product links are bug reports that not only is this
> device non-spec complaint, but it doesn't work.  There seems to be an
> issue with how INTx is de-asserted (or not) leading to interrupt storms
> and requiring the use of irqpoll.  Sure enough, the tulip card I
> installed generated some of these and is operating in polling mode.  The
> threads indicate that these issues are not isolated to Linux and Windows
> users also complain about devices not working or having poor performance
> installed behind this bridge.  All in all, it's an absurdly broken piece
> of hardware.
> 
> I wonder if instead of trying to work around it, we should just
> blacklist the device and ignore that it even exists.  Stop the bus walk
> with some kind of dmesg error and provide a boot time option to scan it.
> It's not the most user friendly option, but a) most people don't seem to
> have anything behind it, b) it barely works if they do.  Thanks,

Having special case quirk makes a lot of sense, but it needs to be done
with care. For people like me who don't use the PCI slots, a simple one
line warning is enough (and blacklist).

Fully ignoring it would probably break users that are attempting to
use the PCI slots. If there are devices on that PCI bus, the kernel should
print out a big warning and engage some sort of fallback like irqpoll.

Re: PCI warning on boot 3.8.0-rc1

[Bjorn Helgaas]

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@redhat.com> 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@redhat.com>
> 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@redhat.com>
> 
> 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, &reg16);
> -	pdev->pcie_flags_reg = reg16;
> -	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
> -	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) {
	...

Bjorn

> +	pdev->pcie_mpss = caps & PCI_EXP_DEVCAP_PAYLOAD;
>  }
>  
>  void set_pcie_hotplug_bridge(struct pci_dev *pdev)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 15472d6..c9ef42c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -300,8 +300,8 @@ struct pci_dev {
>  	unsigned int	msix_enabled:1;
>  	unsigned int	ari_enabled:1;	/* ARI forwarding */
>  	unsigned int	is_managed:1;
> -	unsigned int	is_pcie:1;	/* Obsolete. Will be removed.
> -					   Use pci_is_pcie() instead */
> +	unsigned int	is_pcie:1;	/* Don't access directly,
> +					   use pci_is_pcie() instead */
>  	unsigned int    needs_freset:1; /* Dev requires fundamental reset */
>  	unsigned int	state_saved:1;
>  	unsigned int	is_physfn:1;
> @@ -1689,7 +1689,7 @@ static inline int pci_pcie_cap(struct pci_dev *dev)
>   */
>  static inline bool pci_is_pcie(struct pci_dev *dev)
>  {
> -	return !!pci_pcie_cap(dev);
> +	return dev->is_pcie;
>  }
>  
>  /**

Re: PCI warning on boot 3.8.0-rc1

[Alex Williamson]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > 
> > 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, &reg16);
> > -	pdev->pcie_flags_reg = reg16;
> > -	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
> > -	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,

Alex

Re: PCI warning on boot 3.8.0-rc1

[Bjorn Helgaas]

On Wed, Apr 10, 2013 at 6:01 PM, Alex Williamson
<alex.williamson@redhat.com> 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@redhat.com> 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@redhat.com>
>> > 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@redhat.com>
>> >
>> > 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, &reg16);
>> > -   pdev->pcie_flags_reg = reg16;
>> > -   pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
>> > -   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.

Bjorn

Re: PCI warning on boot 3.8.0-rc1

[Alex Williamson]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> > 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> >
> >> > 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, &reg16);
> >> > -   pdev->pcie_flags_reg = reg16;
> >> > -   pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
> >> > -   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

Re: PCI warning on boot 3.8.0-rc1

[Bjorn Helgaas]

On Mon, Apr 15, 2013 at 1:12 PM, Alex Williamson
<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >> > 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >> >
>> >> > 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, &reg16);
>> >> > -   pdev->pcie_flags_reg = reg16;
>> >> > -   pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
>> >> > -   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,

What good is it to know that "this is really a PCIe device" when it
doesn't have a PCIe capability?  There's no PCIe operation we can
perform on such a device -- we can't determine or set link speed,
control ASPM, use AER, etc.

As far as the rest of the kernel is concerned, I think it's merely a
PCI device.  The requester-ID thing is the only thing I know about
that makes it slightly more than PCI.  That seems like such a special
case thing that it's not worth trying to make a general PCI concept
out of it.

Bjorn