| Message ID | 1360017684.11144.547.camel@bling.home (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, ®16); > - pdev->pcie_flags_reg = reg16; > - pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, ®16); > - pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD; > + pdev->pcie_flags_reg = flags; If we can avoid it, I'd prefer not to complicate the meaning of "pci_is_pcie()" -- it used to mean "this device has a PCIe capability and you can do PCIe things with it." But now it means something else, and we can't do PCIe things with these problem devices anyway. Could we accomplish basically the same thing by making pci_find_upstream_pcie_bridge() look like this? if (pci_is_pcie(pdev)) return NULL; + bridge = pdev->bus->self; + if (bridge && pci_is_pcie(bridge) && + (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT || + pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM)) + return NULL; while (1) { ... 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; > } > > /** -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, ®16); > > - pdev->pcie_flags_reg = reg16; > > - pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, ®16); > > - pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD; > > + pdev->pcie_flags_reg = flags; > > If we can avoid it, I'd prefer not to complicate the meaning of > "pci_is_pcie()" -- it used to mean "this device has a PCIe > capability and you can do PCIe things with it." But now it > means something else, and we can't do PCIe things with these > problem devices anyway. > > Could we accomplish basically the same thing by making > pci_find_upstream_pcie_bridge() look like this? > > if (pci_is_pcie(pdev)) > return NULL; > > + bridge = pdev->bus->self; > + if (bridge && pci_is_pcie(bridge) && > + (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT || > + pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM)) > + return NULL; > > while (1) { > ... This only solves pci_find_upstream_pcie_bridge(3:00.0), I think it still fails for any devices found on subordinate buses below that. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, ®16); >> > - pdev->pcie_flags_reg = reg16; >> > - pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, ®16); >> > - pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD; >> > + pdev->pcie_flags_reg = flags; >> >> If we can avoid it, I'd prefer not to complicate the meaning of >> "pci_is_pcie()" -- it used to mean "this device has a PCIe >> capability and you can do PCIe things with it." But now it >> means something else, and we can't do PCIe things with these >> problem devices anyway. >> >> Could we accomplish basically the same thing by making >> pci_find_upstream_pcie_bridge() look like this? >> >> if (pci_is_pcie(pdev)) >> return NULL; >> >> + bridge = pdev->bus->self; >> + if (bridge && pci_is_pcie(bridge) && >> + (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT || >> + pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM)) >> + return NULL; >> >> while (1) { >> ... > > This only solves pci_find_upstream_pcie_bridge(3:00.0), I think it still > fails for any devices found on subordinate buses below that. Thanks, Can't we apply the same approach throughout the whole tree with some reworking of pci_find_upstream_pcie_bridge()? It seems like pci_find_upstream_pcie_bridge() (and some code in callers) is really trying to figure out the requester-ID for use as the IOMMU's source-ID, but the current code organization seems a bit confusing. I suspect cleaning that up a bit would make it more obvious how to fix this. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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, ®16); > >> > - pdev->pcie_flags_reg = reg16; > >> > - pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, ®16); > >> > - pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD; > >> > + pdev->pcie_flags_reg = flags; > >> > >> If we can avoid it, I'd prefer not to complicate the meaning of > >> "pci_is_pcie()" -- it used to mean "this device has a PCIe > >> capability and you can do PCIe things with it." But now it > >> means something else, and we can't do PCIe things with these > >> problem devices anyway. > >> > >> Could we accomplish basically the same thing by making > >> pci_find_upstream_pcie_bridge() look like this? > >> > >> if (pci_is_pcie(pdev)) > >> return NULL; > >> > >> + bridge = pdev->bus->self; > >> + if (bridge && pci_is_pcie(bridge) && > >> + (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT || > >> + pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM)) > >> + return NULL; > >> > >> while (1) { > >> ... > > > > This only solves pci_find_upstream_pcie_bridge(3:00.0), I think it still > > fails for any devices found on subordinate buses below that. Thanks, > > Can't we apply the same approach throughout the whole tree with some > reworking of pci_find_upstream_pcie_bridge()? > > It seems like pci_find_upstream_pcie_bridge() (and some code in > callers) is really trying to figure out the requester-ID for use as > the IOMMU's source-ID, but the current code organization seems a bit > confusing. I suspect cleaning that up a bit would make it more > obvious how to fix this. But the bug is really that the bridge is a PCIe device but does not expose a PCIe capability. So yes, we can add hacks all around this path to fix it, but we lose the general ability to identify these devices as PCIe. Maybe a compromise is a version of pci_is_pcie() that is a little more flexible, pci_is_probably_pcie()? Then we could use it when we don't actually want to access the capability, but want to test the device type. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 15, 2013 at 1:12 PM, Alex Williamson <alex.williamson@redhat.com> 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@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, ®16); >> >> > - pdev->pcie_flags_reg = reg16; >> >> > - pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, ®16); >> >> > - pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD; >> >> > + pdev->pcie_flags_reg = flags; >> >> >> >> If we can avoid it, I'd prefer not to complicate the meaning of >> >> "pci_is_pcie()" -- it used to mean "this device has a PCIe >> >> capability and you can do PCIe things with it." But now it >> >> means something else, and we can't do PCIe things with these >> >> problem devices anyway. >> >> >> >> Could we accomplish basically the same thing by making >> >> pci_find_upstream_pcie_bridge() look like this? >> >> >> >> if (pci_is_pcie(pdev)) >> >> return NULL; >> >> >> >> + bridge = pdev->bus->self; >> >> + if (bridge && pci_is_pcie(bridge) && >> >> + (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT || >> >> + pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM)) >> >> + return NULL; >> >> >> >> while (1) { >> >> ... >> > >> > This only solves pci_find_upstream_pcie_bridge(3:00.0), I think it still >> > fails for any devices found on subordinate buses below that. Thanks, >> >> Can't we apply the same approach throughout the whole tree with some >> reworking of pci_find_upstream_pcie_bridge()? >> >> It seems like pci_find_upstream_pcie_bridge() (and some code in >> callers) is really trying to figure out the requester-ID for use as >> the IOMMU's source-ID, but the current code organization seems a bit >> confusing. I suspect cleaning that up a bit would make it more >> obvious how to fix this. > > But the bug is really that the bridge is a PCIe device but does not > expose a PCIe capability. So yes, we can add hacks all around this path > to fix it, but we lose the general ability to identify these devices as > PCIe. Maybe a compromise is a version of pci_is_pcie() that is a little > more flexible, pci_is_probably_pcie()? Then we could use it when we > don't actually want to access the capability, but want to test the > device type. Thanks, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, ®16); - pdev->pcie_flags_reg = reg16; - pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, ®16); - pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD; + pdev->pcie_flags_reg = flags; + 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; } /**