| Message ID | 20190415092537.14305-1-kwizart@gmail.com |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: disable nv_msi_ht_cap_quirk_leaf quirk on arm/arm64 | expand |
On Mon, Apr 15, 2019 at 11:25:37AM +0200, Nicolas Chauvet wrote: > This patch disable the use of nv_msi_ht_cap_quirk_leaf quirk on arm and > arm64 NVIDIA devices such as Tegra > > This fixes the following output: > "pci 0000:00:01.0: nv_msi_ht_cap_quirk didn't locate host bridge" > as experienced on a Trimslice device with PCI host bridge enabled > > v3: exclude the quirk for arm and arm64 instead of only for x86 > > v2: use __maybe_unused to avoid a warning on nv_msi_ht_cap_quirk_leaf > > Signed-off-by: Nicolas Chauvet <kwizart@gmail.com> > Reviewed-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > Acked-by: Thierry Reding <treding@nvidia.com> > --- > drivers/pci/quirks.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index a59ad09ce911..1ac65f09d7ee 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -2811,12 +2811,15 @@ static void nv_msi_ht_cap_quirk_all(struct pci_dev *dev) > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); > DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); > > -static void nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) > +static void __maybe_unused nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) > { > return __nv_msi_ht_cap_quirk(dev, 0); > } > +/* HyperTransport is not relevant on theses arches */ > +#if !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64) I don't see any of the previous versions or discussion on the list, so I can't tell what led to this #ifdef, but I'd prefer to avoid #ifdefs like this because they're checking for arch at the wrong level. If you're seeing that warning message, apparently your system contains an Nvidia device that has an HT_CAPTYPE_MSI_MAPPING capability, so HyperTransport is at least in the picture, even on arm/arm64. The quirk assumes a host bridge at 00:00.0, which is really a device-specific assumption, or maybe something specific to HyperTransport. It's certainly not required by the PCI specs, which don't require a host bridge to be visible as a PCI device at all. I'd be tempted to just change that pci_warn() to a pci_info() or maybe even remove it altogether. It would be nice to have the complete "lspci -vv" and dmesg log archived for future reference, since this quirk has such a broad reach (matching PCI_ANY_ID) and it has a long and ugly history already. > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, nv_msi_ht_cap_quirk_leaf); > DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, nv_msi_ht_cap_quirk_leaf); > +#endif > > static void quirk_msi_intx_disable_bug(struct pci_dev *dev) > { > -- > 2.17.2 >
On 15-Apr-19 7:18 PM, Bjorn Helgaas wrote: > On Mon, Apr 15, 2019 at 11:25:37AM +0200, Nicolas Chauvet wrote: >> This patch disable the use of nv_msi_ht_cap_quirk_leaf quirk on arm and >> arm64 NVIDIA devices such as Tegra >> >> This fixes the following output: >> "pci 0000:00:01.0: nv_msi_ht_cap_quirk didn't locate host bridge" >> as experienced on a Trimslice device with PCI host bridge enabled >> >> v3: exclude the quirk for arm and arm64 instead of only for x86 >> >> v2: use __maybe_unused to avoid a warning on nv_msi_ht_cap_quirk_leaf >> >> Signed-off-by: Nicolas Chauvet <kwizart@gmail.com> >> Reviewed-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> >> Acked-by: Thierry Reding <treding@nvidia.com> >> --- >> drivers/pci/quirks.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >> index a59ad09ce911..1ac65f09d7ee 100644 >> --- a/drivers/pci/quirks.c >> +++ b/drivers/pci/quirks.c >> @@ -2811,12 +2811,15 @@ static void nv_msi_ht_cap_quirk_all(struct pci_dev *dev) >> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); >> DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); >> >> -static void nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) >> +static void __maybe_unused nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) >> { >> return __nv_msi_ht_cap_quirk(dev, 0); >> } >> +/* HyperTransport is not relevant on theses arches */ >> +#if !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64) > I don't see any of the previous versions or discussion on the list, so > I can't tell what led to this #ifdef, but I'd prefer to avoid #ifdefs > like this because they're checking for arch at the wrong level. > > If you're seeing that warning message, apparently your system contains > an Nvidia device that has an HT_CAPTYPE_MSI_MAPPING capability, so > HyperTransport is at least in the picture, even on arm/arm64. > > The quirk assumes a host bridge at 00:00.0, which is really a > device-specific assumption, or maybe something specific to > HyperTransport. It's certainly not required by the PCI specs, which > don't require a host bridge to be visible as a PCI device at all. > > I'd be tempted to just change that pci_warn() to a pci_info() or maybe > even remove it altogether. It would be nice to have the complete > "lspci -vv" and dmesg log archived for future reference, since this > quirk has such a broad reach (matching PCI_ANY_ID) and it has a long > and ugly history already. Nvidia HW team confirmed that HyperTransport is supported in AMD CPU + Nvidia NForce motherboard and not supported in Tegra. We are seeing the warn log because Tegra host bridge enumerates from device number 1, as shown in below lspci output taken from Tegra210. HT_CAPTYPE_MSI_MAPPING capability is present in Tegra even though it doesn't support and ARCH check is not best approach here. So changing pci_warn() to pci_info() should be fine. # lspci -D 0000:00:01.0 Class 0604: Device 10de:0fae (rev a1) 0000:00:02.0 Class 0604: Device 10de:0faf (rev a1) 0000:01:00.0 Class 0200: Device 10ec:8168 (rev 0c) 0000:02:00.0 Class 0280: Device 14e4:43ec (rev 02) # lspci -t -[0000:00]-+-01.0-[01]----00.0 \-02.0-[02]----00.0 dmesg log: tegra-pcie 1003000.pcie: 4x1, 1x1 configuration tegra-pcie 1003000.pcie: probing port 0, using 4 lanes tegra-pcie 1003000.pcie: probing port 1, using 1 lanes tegra-pcie 1003000.pcie: PCI host bridge to bus 0000:00 pci_bus 0000:00: root bus resource [io 0x0000-0xffff] pci_bus 0000:00: root bus resource [mem 0x13000000-0x1fffffff] pci_bus 0000:00: root bus resource [mem 0x20000000-0x3fffffff pref] pci_bus 0000:00: root bus resource [bus 00-ff] pci_bus 0000:00: 2-byte config write to 0000:00:01.0 offset 0x4 may corrupt adjacent RW1C bits pci_bus 0000:00: 2-byte config write to 0000:00:01.0 offset 0x4 may corrupt adjacent RW1C bits pci 0000:00:01.0: enabling Extended Tags pci_bus 0000:00: 2-byte config write to 0000:00:01.0 offset 0x88 may corrupt adjacent RW1C bits pci_bus 0000:00: 2-byte config write to 0000:00:01.0 offset 0x3e may corrupt adjacent RW1C bits pci_bus 0000:00: 2-byte config write to 0000:00:01.0 offset 0x52 may corrupt adjacent RW1C bits pci_bus 0000:00: 2-byte config write to 0000:00:01.0 offset 0x4c may corrupt adjacent RW1C bits pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x4 may corrupt adjacent RW1C bits pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x4 may corrupt adjacent RW1C bits pci 0000:00:02.0: enabling Extended Tags pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x88 may corrupt adjacent RW1C bits pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x3e may corrupt adjacent RW1C bits pci 0000:00:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring pci 0000:00:02.0: bridge configuration invalid ([bus 00-00]), reconfiguring pci 0000:00:02.0: BAR 14: assigned [mem 0x13000000-0x135fffff] pci 0000:00:01.0: BAR 14: assigned [mem 0x13600000-0x136fffff] pci 0000:00:01.0: BAR 15: assigned [mem 0x20000000-0x200fffff 64bit pref] pci 0000:00:01.0: BAR 13: assigned [io 0x1000-0x1fff] pci 0000:01:00.0: BAR 4: assigned [mem 0x20000000-0x20003fff 64bit pref] pci 0000:01:00.0: BAR 2: assigned [mem 0x13600000-0x13600fff 64bit] pci 0000:01:00.0: BAR 0: assigned [io 0x1000-0x10ff] pci 0000:00:01.0: PCI bridge to [bus 01] pci 0000:00:01.0: bridge window [io 0x1000-0x1fff] pci 0000:00:01.0: bridge window [mem 0x13600000-0x136fffff] pci 0000:00:01.0: bridge window [mem 0x20000000-0x200fffff 64bit pref] pci 0000:02:00.0: BAR 2: assigned [mem 0x13000000-0x133fffff 64bit] pci 0000:02:00.0: BAR 0: assigned [mem 0x13400000-0x13407fff 64bit] pci 0000:00:02.0: PCI bridge to [bus 02] pci 0000:00:02.0: bridge window [mem 0x13000000-0x135fffff] pci 0000:00:01.0: nv_msi_ht_cap_quirk didn't locate host bridge pcieport 0000:00:01.0: enabling device (0000 -> 0003) pci 0000:00:02.0: nv_msi_ht_cap_quirk didn't locate host bridge pcieport 0000:00:02.0: enabling device (0000 -> 0002) # lspci -vv 00:01.0 Class 0604: Device 10de:0fae (rev a1) 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 Interrupt: pin A routed to IRQ 10 Bus: primary=00, secondary=01, subordinate=01, sec-latency=0 I/O behind bridge: 00001000-00001fff [size=4K] Memory behind bridge: 13600000-136fffff [size=1M] Prefetchable memory behind bridge: 0000000020000000-00000000200fffff [size=1M] 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: [40] Subsystem: Device 10de:0000 Capabilities: [48] Power Management version 3 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Capabilities: [50] MSI: Enable- Count=1/2 Maskable- 64bit+ Address: 0000000179eea000 Data: 0000 Capabilities: [60] HyperTransport: MSI Mapping Enable- Fixed- Mapping Address Base: 00000000fee00000 Capabilities: [80] Express (v2) Root Port (Slot+), MSI 00 DevCap: MaxPayload 128 bytes, PhantFunc 0 ExtTag+ RBE+ DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported- RlxdOrd+ ExtTag+ PhantFunc- AuxPwr- NoSnoop+ MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend- LnkCap: Port #0, Speed 5GT/s, Width x4, ASPM L0s, Exit Latency L0s <512ns ClockPM- Surprise- LLActRep+ BwNot+ ASPMOptComp- LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt+ AutBWInt- LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt- SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surprise- Slot #0, PowerLimit 0.000W; Interlock- NoCompl- SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg- Control: AttnInd Off, PwrInd On, Power- Interlock- SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock- Changed: MRL- PresDet+ LinkState+ RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible- RootCap: CRSVisible- RootSta: PME ReqID 0000, PMEStatus- PMEPending- DevCap2: Completion Timeout: Range AB, TimeoutDis+, LTR+, OBFF Not Supported ARIFwd- AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS- DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR+, OBFF Disabled ARIFwd- AtomicOpsCtl: ReqEn- EgressBlck- LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis- Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS- Compliance De-emphasis: -6dB LnkSta2: Current De-emphasis Level: -3.5dB, EqualizationComplete-, EqualizationPhase1- EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest- Capabilities: [100 v1] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+ AERCap: First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn- MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap- HeaderLog: 00000000 00000000 00000000 00000000 RootCmd: CERptEn- NFERptEn- FERptEn- RootSta: CERcvd- MultCERcvd- UERcvd- MultUERcvd- FirstFatal- NonFatalMsg- FatalMsg- IntMsg 0 ErrorSrc: ERR_COR: 0000 ERR_FATAL/NONFATAL: 0000 Capabilities: [140 v1] L1 PM Substates L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+ PortCommonModeRestoreTime=255us PortTPowerOnTime=10us L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1- T_CommonMode=255us LTR1.2_Threshold=0ns L1SubCtl2: T_PwrOn=10us Kernel driver in use: pcieport 00:02.0 Class 0604: Device 10de:0faf (rev a1) 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 Interrupt: pin A routed to IRQ 10 Bus: primary=00, secondary=02, subordinate=02, sec-latency=0 I/O behind bridge: None Memory behind bridge: 13000000-135fffff [size=6M] Prefetchable memory behind bridge: None 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: [40] Subsystem: Device 10de:0000 Capabilities: [48] Power Management version 3 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Capabilities: [50] MSI: Enable- Count=1/2 Maskable- 64bit+ Address: 0000000179eea000 Data: 0000 Capabilities: [60] HyperTransport: MSI Mapping Enable- Fixed- Mapping Address Base: 00000000fee00000 Capabilities: [80] Express (v2) Root Port (Slot+), MSI 00 DevCap: MaxPayload 128 bytes, PhantFunc 0 ExtTag+ RBE+ DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported- RlxdOrd+ ExtTag+ PhantFunc- AuxPwr- NoSnoop+ MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend- LnkCap: Port #1, Speed 5GT/s, Width x1, ASPM L0s, Exit Latency L0s <512ns ClockPM- Surprise- LLActRep+ BwNot+ ASPMOptComp- LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt+ AutBWInt- LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt- SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surprise- Slot #0, PowerLimit 0.000W; Interlock- NoCompl- SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg- Control: AttnInd Off, PwrInd On, Power- Interlock- SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock- Changed: MRL- PresDet+ LinkState+ RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible- RootCap: CRSVisible- RootSta: PME ReqID 0000, PMEStatus- PMEPending- DevCap2: Completion Timeout: Range AB, TimeoutDis+, LTR+, OBFF Not Supported ARIFwd- AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS- DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR+, OBFF Disabled ARIFwd- AtomicOpsCtl: ReqEn- EgressBlck- LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis- Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS- Compliance De-emphasis: -6dB LnkSta2: Current De-emphasis Level: -3.5dB, EqualizationComplete-, EqualizationPhase1- EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest- Capabilities: [100 v1] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+ AERCap: First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- E[ 916.955543] pci 0000:01:00.0: invalid short VPD tag 00 at offset 1 CRCChkCap+ ECRCChkEn- MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap- HeaderLog: 00000000 00000000 00000000 00000000 RootCmd: CERptEn- NFERptEn- FERptEn- RootSta: CERcvd- MultCERcvd- UERcvd- MultUERcvd- FirstFatal- NonFatalMsg- FatalMsg- IntMsg 0 ErrorSrc: ERR_COR: 0000 ERR_FATAL/NONFATAL: 0000 Capabilities: [140 v1] L1 PM Substates L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+ PortCommonModeRestoreTime=255us PortTPowerOnTime=10us L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1- T_CommonMode=255us LTR1.2_Threshold=0ns L1SubCtl2: T_PwrOn=10us Kernel driver in use: pcieport 01:00.0 Class 0200: Device 10ec:8168 (rev 0c) Subsystem: Device 10ec:0123 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- Interrupt: pin A routed to IRQ 255 Region 0: I/O ports at 1000 [disabled] [size=256] Region 2: Memory at 13600000 (64-bit, non-prefetchable) [disabled] [size=4K] Region 4: Memory at 20000000 (64-bit, prefetchable) [disabled] [size=16K] Capabilities: [40] Power Management version 3 Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+) Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME- Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ Address: 0000000000000000 Data: 0000 Capabilities: [70] Express (v2) Endpoint, MSI 01 DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset- SlotPowerLimit 0.000W DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported- RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop- MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend- LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s unlimited, L1 <64us ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+ LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt- DevCap2: Completion Timeout: Range ABCD, TimeoutDis+, LTR+, OBFF Via message/WAKE# AtomicOpsCap: 32bit- 64bit- 128bitCAS- DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR-, OBFF Disabled AtomicOpsCtl: ReqEn- LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis- Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS- Compliance De-emphasis: -6dB LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-, EqualizationPhase1- EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest- Capabilities: [b0] MSI-X: Enable- Count=4 Masked- Vector table: BAR=4 offset=00000000 PBA: BAR=4 offset=00000800 Capabilities: [d0] Vital Product Data pcilib: sysfs_read_vpd: read failed: Input/output error Not readable Capabilities: [100 v1] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+ AERCap: First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn- MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap- HeaderLog: 00000000 00000000 00000000 00000000 Capabilities: [140 v1] Virtual Channel Caps: LPEVC=0 RefClk=100ns PATEntryBits=1 Arb: Fixed- WRR32- WRR64- WRR128- Ctrl: ArbSelect=Fixed Status: InProgress- VC0: Caps: PATOffset=00 MaxTimeSlots=1 RejSnoopTrans- Arb: Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256- Ctrl: Enable+ ID=0 ArbSelect=Fixed TC/VC=ff Status: NegoPending- InProgress- Capabilities: [160 v1] Device Serial Number 01-00-00-00-68-4c-e0-00 Capabilities: [170 v1] Latency Tolerance Reporting Max snoop latency: 0ns Max no snoop latency: 0ns 02:00.0 Class 0280: Device 14e4:43ec (rev 02) Subsystem: Device 17aa:0777 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- Interrupt: pin A routed to IRQ 0 Region 0: Memory at 13400000 (64-bit, non-prefetchable) [disabled] [size=32K] Region 2: Memory at 13000000 (64-bit, non-prefetchable) [disabled] [size=4M] Capabilities: [48] Power Management version 3 Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+) Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=2 PME- Capabilities: [58] MSI: Enable- Count=1/16 Maskable- 64bit+ Address: 0000000000000000 Data: 0000 Capabilities: [68] Vendor Specific Information: Len=44 <?> Capabilities: [ac] Express (v2) Endpoint, MSI 00 DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s <4us, L1 unlimited ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset- SlotPowerLimit 0.000W DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported- RlxdOrd+ ExtTag- PhantFunc- AuxPwr+ NoSnoop+ MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend- LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <2us, L1 <32us ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+ LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt- DevCap2: Completion Timeout: Range ABCD, TimeoutDis+, LTR+, OBFF Via WAKE# AtomicOpsCap: 32bit- 64bit- 128bitCAS- DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR-, OBFF Disabled AtomicOpsCtl: ReqEn- LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis- Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS- Compliance De-emphasis: -6dB LnkSta2: Current De-emphasis Level: -3.5dB, EqualizationComplete-, EqualizationPhase1- EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest- Capabilities: [100 v1] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+ AERCap: First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn- MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap- HeaderLog: 00000000 00000000 00000000 00000000 Capabilities: [13c v1] Device Serial Number 00-00-a2-ff-ff-75-5c-93 Capabilities: [150 v1] Power Budgeting <?> Capabilities: [160 v1] Virtual Channel Caps: LPEVC=0 RefClk=100ns PATEntryBits=1 Arb: Fixed- WRR32- WRR64- WRR128- Ctrl: ArbSelect=Fixed Status: InProgress- VC0: Caps: PATOffset=00 MaxTimeSlots=1 RejSnoopTrans- Arb: Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256- Ctrl: Enable+ ID=0 ArbSelect=Fixed TC/VC=ff Status: NegoPending- InProgress- Capabilities: [1b0 v1] Latency Tolerance Reporting Max snoop latency: 0ns Max no snoop latency: 0ns Capabilities: [220 v1] #15 Capabilities: [240 v1] L1 PM Substates L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+ PortCommonModeRestoreTime=0us PortTPowerOnTime=50us L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1- T_CommonMode=0us LTR1.2_Threshold=0ns L1SubCtl2: T_PwrOn=10us Manikanta >> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, nv_msi_ht_cap_quirk_leaf); >> DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, nv_msi_ht_cap_quirk_leaf); >> +#endif >> >> static void quirk_msi_intx_disable_bug(struct pci_dev *dev) >> { >> -- >> 2.17.2 >>
On Mon, Apr 15, 2019 at 08:48:13AM -0500, Bjorn Helgaas wrote: > On Mon, Apr 15, 2019 at 11:25:37AM +0200, Nicolas Chauvet wrote: > > This patch disable the use of nv_msi_ht_cap_quirk_leaf quirk on arm and > > arm64 NVIDIA devices such as Tegra > > > > This fixes the following output: > > "pci 0000:00:01.0: nv_msi_ht_cap_quirk didn't locate host bridge" > > as experienced on a Trimslice device with PCI host bridge enabled > > > > v3: exclude the quirk for arm and arm64 instead of only for x86 > > > > v2: use __maybe_unused to avoid a warning on nv_msi_ht_cap_quirk_leaf > > > > Signed-off-by: Nicolas Chauvet <kwizart@gmail.com> > > Reviewed-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > > Acked-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/pci/quirks.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index a59ad09ce911..1ac65f09d7ee 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -2811,12 +2811,15 @@ static void nv_msi_ht_cap_quirk_all(struct pci_dev *dev) > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); > > DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); > > > > -static void nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) > > +static void __maybe_unused nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) > > { > > return __nv_msi_ht_cap_quirk(dev, 0); > > } > > +/* HyperTransport is not relevant on theses arches */ > > +#if !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64) > > I don't see any of the previous versions or discussion on the list, so > I can't tell what led to this #ifdef, but I'd prefer to avoid #ifdefs > like this because they're checking for arch at the wrong level. > > If you're seeing that warning message, apparently your system contains > an Nvidia device that has an HT_CAPTYPE_MSI_MAPPING capability, so > HyperTransport is at least in the picture, even on arm/arm64. > > The quirk assumes a host bridge at 00:00.0, which is really a > device-specific assumption, or maybe something specific to > HyperTransport. It's certainly not required by the PCI specs, which > don't require a host bridge to be visible as a PCI device at all. > > I'd be tempted to just change that pci_warn() to a pci_info() or maybe > even remove it altogether. It would be nice to have the complete > "lspci -vv" and dmesg log archived for future reference, since this > quirk has such a broad reach (matching PCI_ANY_ID) and it has a long > and ugly history already. I've been thinking about this a bit and it seems to me like the intention of the quirk is to make sure that all devices have their HT MSI mapping capabilities disabled if the system doesn't support HT. As you said, the fact that a host bridge may not be present isn't an error, however, it also means that we have to assume that HT is not supported in such cases and make sure the HT MSI mapping capability gets disabled for all devices. The following patch tries to address this and gets rid of the warning on Tegra systems. Thoughts? Thierry --- >8 --- From 69fb3b269cc852964385436694d79765d5c1cab3 Mon Sep 17 00:00:00 2001 From: Thierry Reding <treding@nvidia.com> Date: Tue, 16 Apr 2019 13:27:53 +0200 Subject: [PATCH] PCI: Be less noisy if no HT host bridge exists Embedded systems will typically not expose a host bridge's configuration space, so such setups must be assumed not to support HyperTransport, and consequently the HT MSI mapping capability should be disabled for all devices. Signed-off-by: Thierry Reding <treding@nvidia.com> --- drivers/pci/quirks.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index f9cd4d432e05..705891c88073 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -2792,24 +2792,28 @@ static void __nv_msi_ht_cap_quirk(struct pci_dev *dev, int all) */ host_bridge = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus), 0, PCI_DEVFN(0, 0)); - if (host_bridge == NULL) { - pci_warn(dev, "nv_msi_ht_cap_quirk didn't locate host bridge\n"); - return; - } - - pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE); - if (pos != 0) { - /* Host bridge is to HT */ - if (found == 1) { - /* it is not enabled, try to enable it */ - if (all) - ht_enable_msi_mapping(dev); - else - nv_ht_enable_msi_mapping(dev); + if (host_bridge) { + pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE); + if (pos != 0) { + /* Host bridge is to HT */ + if (found == 1) { + /* it is not enabled, try to enable it */ + if (all) + ht_enable_msi_mapping(dev); + else + nv_ht_enable_msi_mapping(dev); + } + goto out; } - goto out; } + /* + * Some systems, such as embedded devices, may not expose the host + * bridge's configuration space. Assume such systems to not support + * HyperTransport and disable the HT MSI mapping capability for all + * devices. + */ + /* HT MSI is not enabled */ if (found == 1) goto out;
On 16-Apr-19 5:05 PM, Thierry Reding wrote: > On Mon, Apr 15, 2019 at 08:48:13AM -0500, Bjorn Helgaas wrote: >> On Mon, Apr 15, 2019 at 11:25:37AM +0200, Nicolas Chauvet wrote: >>> This patch disable the use of nv_msi_ht_cap_quirk_leaf quirk on arm and >>> arm64 NVIDIA devices such as Tegra >>> >>> This fixes the following output: >>> "pci 0000:00:01.0: nv_msi_ht_cap_quirk didn't locate host bridge" >>> as experienced on a Trimslice device with PCI host bridge enabled >>> >>> v3: exclude the quirk for arm and arm64 instead of only for x86 >>> >>> v2: use __maybe_unused to avoid a warning on nv_msi_ht_cap_quirk_leaf >>> >>> Signed-off-by: Nicolas Chauvet <kwizart@gmail.com> >>> Reviewed-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> >>> Acked-by: Thierry Reding <treding@nvidia.com> >>> --- >>> drivers/pci/quirks.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >>> index a59ad09ce911..1ac65f09d7ee 100644 >>> --- a/drivers/pci/quirks.c >>> +++ b/drivers/pci/quirks.c >>> @@ -2811,12 +2811,15 @@ static void nv_msi_ht_cap_quirk_all(struct pci_dev *dev) >>> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); >>> DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); >>> >>> -static void nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) >>> +static void __maybe_unused nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) >>> { >>> return __nv_msi_ht_cap_quirk(dev, 0); >>> } >>> +/* HyperTransport is not relevant on theses arches */ >>> +#if !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64) >> I don't see any of the previous versions or discussion on the list, so >> I can't tell what led to this #ifdef, but I'd prefer to avoid #ifdefs >> like this because they're checking for arch at the wrong level. >> >> If you're seeing that warning message, apparently your system contains >> an Nvidia device that has an HT_CAPTYPE_MSI_MAPPING capability, so >> HyperTransport is at least in the picture, even on arm/arm64. >> >> The quirk assumes a host bridge at 00:00.0, which is really a >> device-specific assumption, or maybe something specific to >> HyperTransport. It's certainly not required by the PCI specs, which >> don't require a host bridge to be visible as a PCI device at all. >> >> I'd be tempted to just change that pci_warn() to a pci_info() or maybe >> even remove it altogether. It would be nice to have the complete >> "lspci -vv" and dmesg log archived for future reference, since this >> quirk has such a broad reach (matching PCI_ANY_ID) and it has a long >> and ugly history already. > I've been thinking about this a bit and it seems to me like the > intention of the quirk is to make sure that all devices have their HT > MSI mapping capabilities disabled if the system doesn't support HT. > As you said, the fact that a host bridge may not be present isn't an > error, however, it also means that we have to assume that HT is not > supported in such cases and make sure the HT MSI mapping capability gets > disabled for all devices. > > The following patch tries to address this and gets rid of the warning on > Tegra systems. > > Thoughts? > > Thierry > --- >8 --- > From 69fb3b269cc852964385436694d79765d5c1cab3 Mon Sep 17 00:00:00 2001 > From: Thierry Reding <treding@nvidia.com> > Date: Tue, 16 Apr 2019 13:27:53 +0200 > Subject: [PATCH] PCI: Be less noisy if no HT host bridge exists > > Embedded systems will typically not expose a host bridge's configuration > space, so such setups must be assumed not to support HyperTransport, and > consequently the HT MSI mapping capability should be disabled for all > devices. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/pci/quirks.c | 34 +++++++++++++++++++--------------- > 1 file changed, 19 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index f9cd4d432e05..705891c88073 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -2792,24 +2792,28 @@ static void __nv_msi_ht_cap_quirk(struct pci_dev *dev, int all) > */ > host_bridge = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus), 0, > PCI_DEVFN(0, 0)); > - if (host_bridge == NULL) { > - pci_warn(dev, "nv_msi_ht_cap_quirk didn't locate host bridge\n"); > - return; > - } > - > - pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE); > - if (pos != 0) { > - /* Host bridge is to HT */ > - if (found == 1) { > - /* it is not enabled, try to enable it */ > - if (all) > - ht_enable_msi_mapping(dev); > - else > - nv_ht_enable_msi_mapping(dev); > + if (host_bridge) { > + pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE); > + if (pos != 0) { > + /* Host bridge is to HT */ > + if (found == 1) { > + /* it is not enabled, try to enable it */ > + if (all) > + ht_enable_msi_mapping(dev); > + else > + nv_ht_enable_msi_mapping(dev); > + } > + goto out; > } > - goto out; > } > > + /* > + * Some systems, such as embedded devices, may not expose the host > + * bridge's configuration space. Assume such systems to not support > + * HyperTransport and disable the HT MSI mapping capability for all > + * devices. > + */ > + > /* HT MSI is not enabled */ > if (found == 1) > goto out; I verified this patch on Tegra210. Reviewed-by: Manikanta Maddireddy <mmaddireddy@nvidia.com <mailto:mmaddireddy@nvidia.com>> Tested-by: Manikanta Maddireddy <mmaddireddy@nvidia.com <mailto:mmaddireddy@nvidia.com>>
On Tue, Apr 16, 2019 at 01:35:04PM +0200, Thierry Reding wrote: > On Mon, Apr 15, 2019 at 08:48:13AM -0500, Bjorn Helgaas wrote: > > On Mon, Apr 15, 2019 at 11:25:37AM +0200, Nicolas Chauvet wrote: > > > This patch disable the use of nv_msi_ht_cap_quirk_leaf quirk on arm and > > > arm64 NVIDIA devices such as Tegra > > > > > > This fixes the following output: > > > "pci 0000:00:01.0: nv_msi_ht_cap_quirk didn't locate host bridge" > > > as experienced on a Trimslice device with PCI host bridge enabled > > > > > > v3: exclude the quirk for arm and arm64 instead of only for x86 > > > > > > v2: use __maybe_unused to avoid a warning on nv_msi_ht_cap_quirk_leaf > > > > > > Signed-off-by: Nicolas Chauvet <kwizart@gmail.com> > > > Reviewed-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > > > Acked-by: Thierry Reding <treding@nvidia.com> > > > --- > > > drivers/pci/quirks.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > > index a59ad09ce911..1ac65f09d7ee 100644 > > > --- a/drivers/pci/quirks.c > > > +++ b/drivers/pci/quirks.c > > > @@ -2811,12 +2811,15 @@ static void nv_msi_ht_cap_quirk_all(struct pci_dev *dev) > > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); > > > DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); > > > > > > -static void nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) > > > +static void __maybe_unused nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) > > > { > > > return __nv_msi_ht_cap_quirk(dev, 0); > > > } > > > +/* HyperTransport is not relevant on theses arches */ > > > +#if !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64) > > > > I don't see any of the previous versions or discussion on the list, so > > I can't tell what led to this #ifdef, but I'd prefer to avoid #ifdefs > > like this because they're checking for arch at the wrong level. > > > > If you're seeing that warning message, apparently your system contains > > an Nvidia device that has an HT_CAPTYPE_MSI_MAPPING capability, so > > HyperTransport is at least in the picture, even on arm/arm64. > > > > The quirk assumes a host bridge at 00:00.0, which is really a > > device-specific assumption, or maybe something specific to > > HyperTransport. It's certainly not required by the PCI specs, which > > don't require a host bridge to be visible as a PCI device at all. > > > > I'd be tempted to just change that pci_warn() to a pci_info() or maybe > > even remove it altogether. It would be nice to have the complete > > "lspci -vv" and dmesg log archived for future reference, since this > > quirk has such a broad reach (matching PCI_ANY_ID) and it has a long > > and ugly history already. > > I've been thinking about this a bit and it seems to me like the > intention of the quirk is to make sure that all devices have their HT > MSI mapping capabilities disabled if the system doesn't support HT. > As you said, the fact that a host bridge may not be present isn't an > error, however, it also means that we have to assume that HT is not > supported in such cases and make sure the HT MSI mapping capability gets > disabled for all devices. > > The following patch tries to address this and gets rid of the warning on > Tegra systems. One thing I still don't understand here is whether this quirk is actually for a defect in Nvidia and ALi endpoints, or if it's related to an issue in certain host bridges, or if it's just a generic HT-related thing that's correctly implemented in hardware but Linux doesn't know how to deal with. Having this quirk run forever and ever on every Nvidia and ALI endpoint just seems like massive overkill. We assume a host bridge must be xxxx:00:00.0, which I don't think is true in HT. __nv_msi_ht_cap_quirk() doesn't look at the topology at all, so it apparently assumes any device in domain xxxx with an HT MSI capability must be part of the xxxx:00:00.0 HT hierarchy. I don't think that's true either. It seems like what we really want to do is look for HT_CAPTYPE_SLAVE in generic code for every device we enumerate and set an "ht_tree" bit in its pci_dev if we find it. Then inherit that for all children, and use that to enable/disable HT MSI mapping. I bet there's no actual hardware defect here that we're working around. I suspect this could all be done in generic code as above. Maybe we think we only need this for Nvidia/ALi because of differences in how BIOS leaves things set up. And that probably depends mostly on the host bridge, not the endpoints that the quirks apply to. Sigh. I suppose it's probably not worth trying to unravel all this at this stage of the game. The commits below have lots of interesting information if anybody *were* to dig into this: de7453065d5d ("PCI: don't enable too much HT MSI mapping") 1dec6b054dd1 ("PCI: don't enable too many HT MSI mappings") 439a7733e8fc ("PCI: enable nv_msi_ht_cap_quirk for ALi bridges") 9dc625e72309 ("PCI: quirks: set 'En' bit of MSI Mapping for devices onHT-based nvidia platform") 6bae1d96c6d7 ("PCI: quirk: enable MSI Mapping on HT1000") 1d84b5424efb ("PCI: Add MSI quirk for ServerWorks HT1000 PCIX bridge.") e3008dedff4b ("PCI: disable MSI by default on systems with Serverworks HT1000 chips") 6397c75cbc4d ("MSI: Blacklist PCI-E chipsets depending on Hypertransport MSI capability") Bjorn > --- >8 --- > From 69fb3b269cc852964385436694d79765d5c1cab3 Mon Sep 17 00:00:00 2001 > From: Thierry Reding <treding@nvidia.com> > Date: Tue, 16 Apr 2019 13:27:53 +0200 > Subject: [PATCH] PCI: Be less noisy if no HT host bridge exists > > Embedded systems will typically not expose a host bridge's configuration > space, so such setups must be assumed not to support HyperTransport, and > consequently the HT MSI mapping capability should be disabled for all > devices. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/pci/quirks.c | 34 +++++++++++++++++++--------------- > 1 file changed, 19 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index f9cd4d432e05..705891c88073 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -2792,24 +2792,28 @@ static void __nv_msi_ht_cap_quirk(struct pci_dev *dev, int all) > */ > host_bridge = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus), 0, > PCI_DEVFN(0, 0)); > - if (host_bridge == NULL) { > - pci_warn(dev, "nv_msi_ht_cap_quirk didn't locate host bridge\n"); > - return; > - } > - > - pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE); > - if (pos != 0) { > - /* Host bridge is to HT */ > - if (found == 1) { > - /* it is not enabled, try to enable it */ > - if (all) > - ht_enable_msi_mapping(dev); > - else > - nv_ht_enable_msi_mapping(dev); > + if (host_bridge) { > + pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE); > + if (pos != 0) { > + /* Host bridge is to HT */ > + if (found == 1) { > + /* it is not enabled, try to enable it */ > + if (all) > + ht_enable_msi_mapping(dev); > + else > + nv_ht_enable_msi_mapping(dev); > + } > + goto out; > } > - goto out; > } > > + /* > + * Some systems, such as embedded devices, may not expose the host > + * bridge's configuration space. Assume such systems to not support > + * HyperTransport and disable the HT MSI mapping capability for all > + * devices. > + */ > + > /* HT MSI is not enabled */ > if (found == 1) > goto out; > -- > 2.21.0 >
On Tue, Apr 16, 2019 at 01:35:04PM +0200, Thierry Reding wrote: > On Mon, Apr 15, 2019 at 08:48:13AM -0500, Bjorn Helgaas wrote: > > On Mon, Apr 15, 2019 at 11:25:37AM +0200, Nicolas Chauvet wrote: > > > This patch disable the use of nv_msi_ht_cap_quirk_leaf quirk on arm and > > > arm64 NVIDIA devices such as Tegra > > > > > > This fixes the following output: > > > "pci 0000:00:01.0: nv_msi_ht_cap_quirk didn't locate host bridge" > > > as experienced on a Trimslice device with PCI host bridge enabled > > > > > > v3: exclude the quirk for arm and arm64 instead of only for x86 > > > > > > v2: use __maybe_unused to avoid a warning on nv_msi_ht_cap_quirk_leaf > > > > > > Signed-off-by: Nicolas Chauvet <kwizart@gmail.com> > > > Reviewed-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > > > Acked-by: Thierry Reding <treding@nvidia.com> > > > --- > > > drivers/pci/quirks.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > > index a59ad09ce911..1ac65f09d7ee 100644 > > > --- a/drivers/pci/quirks.c > > > +++ b/drivers/pci/quirks.c > > > @@ -2811,12 +2811,15 @@ static void nv_msi_ht_cap_quirk_all(struct pci_dev *dev) > > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); > > > DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); > > > > > > -static void nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) > > > +static void __maybe_unused nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) > > > { > > > return __nv_msi_ht_cap_quirk(dev, 0); > > > } > > > +/* HyperTransport is not relevant on theses arches */ > > > +#if !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64) > > > > I don't see any of the previous versions or discussion on the list, so > > I can't tell what led to this #ifdef, but I'd prefer to avoid #ifdefs > > like this because they're checking for arch at the wrong level. > > > > If you're seeing that warning message, apparently your system contains > > an Nvidia device that has an HT_CAPTYPE_MSI_MAPPING capability, so > > HyperTransport is at least in the picture, even on arm/arm64. > > > > The quirk assumes a host bridge at 00:00.0, which is really a > > device-specific assumption, or maybe something specific to > > HyperTransport. It's certainly not required by the PCI specs, which > > don't require a host bridge to be visible as a PCI device at all. > > > > I'd be tempted to just change that pci_warn() to a pci_info() or maybe > > even remove it altogether. It would be nice to have the complete > > "lspci -vv" and dmesg log archived for future reference, since this > > quirk has such a broad reach (matching PCI_ANY_ID) and it has a long > > and ugly history already. > > I've been thinking about this a bit and it seems to me like the > intention of the quirk is to make sure that all devices have their HT > MSI mapping capabilities disabled if the system doesn't support HT. > As you said, the fact that a host bridge may not be present isn't an > error, however, it also means that we have to assume that HT is not > supported in such cases and make sure the HT MSI mapping capability gets > disabled for all devices. > > The following patch tries to address this and gets rid of the warning on > Tegra systems. Also interesting that Nicolas' original patch took this quirk completely out of the picture for ARM/ARM64, while your patch actively disables HT MSI mapping if there's no HT host bridge. If HT MSI Mapping Capability is enabled, the device is supposed to map MSIs into HyperTransport interrupt messages (HyperTransport I/O Link spec r3.10e, sec 7.12). I don't know what that even means if the device is on a PCIe link instead of an HT chain. > --- >8 --- > From 69fb3b269cc852964385436694d79765d5c1cab3 Mon Sep 17 00:00:00 2001 > From: Thierry Reding <treding@nvidia.com> > Date: Tue, 16 Apr 2019 13:27:53 +0200 > Subject: [PATCH] PCI: Be less noisy if no HT host bridge exists > > Embedded systems will typically not expose a host bridge's configuration > space, so such setups must be assumed not to support HyperTransport, and > consequently the HT MSI mapping capability should be disabled for all > devices. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/pci/quirks.c | 34 +++++++++++++++++++--------------- > 1 file changed, 19 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index f9cd4d432e05..705891c88073 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -2792,24 +2792,28 @@ static void __nv_msi_ht_cap_quirk(struct pci_dev *dev, int all) > */ > host_bridge = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus), 0, > PCI_DEVFN(0, 0)); > - if (host_bridge == NULL) { > - pci_warn(dev, "nv_msi_ht_cap_quirk didn't locate host bridge\n"); > - return; > - } > - > - pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE); > - if (pos != 0) { > - /* Host bridge is to HT */ > - if (found == 1) { > - /* it is not enabled, try to enable it */ > - if (all) > - ht_enable_msi_mapping(dev); > - else > - nv_ht_enable_msi_mapping(dev); > + if (host_bridge) { > + pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE); > + if (pos != 0) { > + /* Host bridge is to HT */ > + if (found == 1) { > + /* it is not enabled, try to enable it */ > + if (all) > + ht_enable_msi_mapping(dev); > + else > + nv_ht_enable_msi_mapping(dev); > + } > + goto out; > } > - goto out; > } > > + /* > + * Some systems, such as embedded devices, may not expose the host > + * bridge's configuration space. Assume such systems to not support > + * HyperTransport and disable the HT MSI mapping capability for all > + * devices. > + */ > + > /* HT MSI is not enabled */ > if (found == 1) > goto out; > -- > 2.21.0 >
On Thu, Apr 18, 2019 at 06:17:04PM -0500, Bjorn Helgaas wrote: > On Tue, Apr 16, 2019 at 01:35:04PM +0200, Thierry Reding wrote: > > On Mon, Apr 15, 2019 at 08:48:13AM -0500, Bjorn Helgaas wrote: > > > On Mon, Apr 15, 2019 at 11:25:37AM +0200, Nicolas Chauvet wrote: > > > > This patch disable the use of nv_msi_ht_cap_quirk_leaf quirk on arm and > > > > arm64 NVIDIA devices such as Tegra > > > > > > > > This fixes the following output: > > > > "pci 0000:00:01.0: nv_msi_ht_cap_quirk didn't locate host bridge" > > > > as experienced on a Trimslice device with PCI host bridge enabled > > > > > > > > v3: exclude the quirk for arm and arm64 instead of only for x86 > > > > > > > > v2: use __maybe_unused to avoid a warning on nv_msi_ht_cap_quirk_leaf > > > > > > > > Signed-off-by: Nicolas Chauvet <kwizart@gmail.com> > > > > Reviewed-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > > > > Acked-by: Thierry Reding <treding@nvidia.com> > > > > --- > > > > drivers/pci/quirks.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > > > index a59ad09ce911..1ac65f09d7ee 100644 > > > > --- a/drivers/pci/quirks.c > > > > +++ b/drivers/pci/quirks.c > > > > @@ -2811,12 +2811,15 @@ static void nv_msi_ht_cap_quirk_all(struct pci_dev *dev) > > > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); > > > > DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); > > > > > > > > -static void nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) > > > > +static void __maybe_unused nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) > > > > { > > > > return __nv_msi_ht_cap_quirk(dev, 0); > > > > } > > > > +/* HyperTransport is not relevant on theses arches */ > > > > +#if !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64) > > > > > > I don't see any of the previous versions or discussion on the list, so > > > I can't tell what led to this #ifdef, but I'd prefer to avoid #ifdefs > > > like this because they're checking for arch at the wrong level. > > > > > > If you're seeing that warning message, apparently your system contains > > > an Nvidia device that has an HT_CAPTYPE_MSI_MAPPING capability, so > > > HyperTransport is at least in the picture, even on arm/arm64. > > > > > > The quirk assumes a host bridge at 00:00.0, which is really a > > > device-specific assumption, or maybe something specific to > > > HyperTransport. It's certainly not required by the PCI specs, which > > > don't require a host bridge to be visible as a PCI device at all. > > > > > > I'd be tempted to just change that pci_warn() to a pci_info() or maybe > > > even remove it altogether. It would be nice to have the complete > > > "lspci -vv" and dmesg log archived for future reference, since this > > > quirk has such a broad reach (matching PCI_ANY_ID) and it has a long > > > and ugly history already. > > > > I've been thinking about this a bit and it seems to me like the > > intention of the quirk is to make sure that all devices have their HT > > MSI mapping capabilities disabled if the system doesn't support HT. > > As you said, the fact that a host bridge may not be present isn't an > > error, however, it also means that we have to assume that HT is not > > supported in such cases and make sure the HT MSI mapping capability gets > > disabled for all devices. > > > > The following patch tries to address this and gets rid of the warning on > > Tegra systems. > > Also interesting that Nicolas' original patch took this quirk > completely out of the picture for ARM/ARM64, while your patch actively > disables HT MSI mapping if there's no HT host bridge. I should note here that this is just my interpretation of what I think the quirk was originally meant to be. I don't have any hardware to test this on, nor do I know much (well, nothing, to be frank) about Hyper- Transport. So my thinking was that if there's no HT host bridge, then it doesn't make any sense to enable HT MSI mapping either, which I think may have been the intent of this quirk: make sure that if the system is not HT capable, that none of the PCI devices try to use HT MSI mapping. Thierry > If HT MSI Mapping Capability is enabled, the device is supposed to map > MSIs into HyperTransport interrupt messages (HyperTransport I/O Link > spec r3.10e, sec 7.12). I don't know what that even means if the > device is on a PCIe link instead of an HT chain. > > > --- >8 --- > > From 69fb3b269cc852964385436694d79765d5c1cab3 Mon Sep 17 00:00:00 2001 > > From: Thierry Reding <treding@nvidia.com> > > Date: Tue, 16 Apr 2019 13:27:53 +0200 > > Subject: [PATCH] PCI: Be less noisy if no HT host bridge exists > > > > Embedded systems will typically not expose a host bridge's configuration > > space, so such setups must be assumed not to support HyperTransport, and > > consequently the HT MSI mapping capability should be disabled for all > > devices. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/pci/quirks.c | 34 +++++++++++++++++++--------------- > > 1 file changed, 19 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index f9cd4d432e05..705891c88073 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -2792,24 +2792,28 @@ static void __nv_msi_ht_cap_quirk(struct pci_dev *dev, int all) > > */ > > host_bridge = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus), 0, > > PCI_DEVFN(0, 0)); > > - if (host_bridge == NULL) { > > - pci_warn(dev, "nv_msi_ht_cap_quirk didn't locate host bridge\n"); > > - return; > > - } > > - > > - pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE); > > - if (pos != 0) { > > - /* Host bridge is to HT */ > > - if (found == 1) { > > - /* it is not enabled, try to enable it */ > > - if (all) > > - ht_enable_msi_mapping(dev); > > - else > > - nv_ht_enable_msi_mapping(dev); > > + if (host_bridge) { > > + pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE); > > + if (pos != 0) { > > + /* Host bridge is to HT */ > > + if (found == 1) { > > + /* it is not enabled, try to enable it */ > > + if (all) > > + ht_enable_msi_mapping(dev); > > + else > > + nv_ht_enable_msi_mapping(dev); > > + } > > + goto out; > > } > > - goto out; > > } > > > > + /* > > + * Some systems, such as embedded devices, may not expose the host > > + * bridge's configuration space. Assume such systems to not support > > + * HyperTransport and disable the HT MSI mapping capability for all > > + * devices. > > + */ > > + > > /* HT MSI is not enabled */ > > if (found == 1) > > goto out; > > -- > > 2.21.0 > > > >
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index a59ad09ce911..1ac65f09d7ee 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -2811,12 +2811,15 @@ static void nv_msi_ht_cap_quirk_all(struct pci_dev *dev) DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all); -static void nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) +static void __maybe_unused nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev) { return __nv_msi_ht_cap_quirk(dev, 0); } +/* HyperTransport is not relevant on theses arches */ +#if !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64) DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, nv_msi_ht_cap_quirk_leaf); DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, nv_msi_ht_cap_quirk_leaf); +#endif static void quirk_msi_intx_disable_bug(struct pci_dev *dev) {