| Message ID | 20210112072739.31624-1-mingchuang.qiao@mediatek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pci: avoid unsync of LTR mechanism configuration | expand |
Note subject line tips at https://lore.kernel.org/r/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com On Tue, Jan 12, 2021 at 03:27:39PM +0800, mingchuang.qiao@mediatek.com wrote: > From: Mingchuang Qiao <mingchuang.qiao@mediatek.com> > > In pci bus scan flow, the LTR mechanism enable bit of DEVCTL2 register > is configured in pci_configure_ltr(). If device and it's bridge both > support LTR mechanism, LTR mechanism of device and it's bridge will > be enabled in DEVCTL2 register. And the flag pci_dev->ltr_path will > be set as 1. s/it's/its/ twice above. It's == It is. Its == belonging to 'it'. Weird, I know, but that's English for you :) > For some pcie products, pcie link becomes down when device reset. And then > the LTR mechanism enable bit of bridge will become 0 based on description > in PCIE r4.0, sec 7.8.16. However, the pci_dev->ltr_path value of bridge > is still 1. Remove and rescan flow could be triggered to recover after > device reset. In the bus rescan flow, the LTR mechanism of device will be > enabled in pci_configure_ltr() due to ltr_path of its bridge is still 1. s/pcie/PCIe/ twice above. s/PCIE/PCIe/; also reference r5.0 instead of r4.0 if possible. This sounds like a general problem of most device config bits being cleared by reset. Usually these are restored by pci_restore_state(). Is that function missing a restore for PCI_EXP_DEVCTL2? I'd rather have a general-purpose way of restoring all the config state than little pieces scattered all over. > When device's LTR mechanism is enabled, device will send LTR message to > bridge. Bridge receives the device's LTR message and found bridge's LTR > mechanism is disabled. Then the bridge will generate unsupported request > and other error handling flow will be triggered such as AER Non-Fatal > error handling. > > This patch is used to avoid this unsupported request and make the bridge's > ltr_path value is aligned with DEVCTL2 register value. Check bridge > register value if aligned with ltr_path in pci_configure_ltr(). If > register value is disable and the ltr_path is 1, we need configure > the register value as enable. > > Signed-off-by: Mingchuang Qiao <mingchuang.qiao@mediatek.com> > --- > drivers/pci/probe.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 953f15abc850..49355cf4af54 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2132,9 +2132,21 @@ static void pci_configure_ltr(struct pci_dev *dev) > * Complex and all intermediate Switches indicate support for LTR. > * PCIe r4.0, sec 6.18. > */ > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || > - ((bridge = pci_upstream_bridge(dev)) && > - bridge->ltr_path)) { > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) { > + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, > + PCI_EXP_DEVCTL2_LTR_EN); > + dev->ltr_path = 1; > + return; > + } > + > + bridge = pci_upstream_bridge(dev); > + if (bridge && bridge->ltr_path) { > + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl); > + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) { > + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2, > + PCI_EXP_DEVCTL2_LTR_EN); > + } > + > pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, > PCI_EXP_DEVCTL2_LTR_EN); > dev->ltr_path = 1; > -- > 2.18.0 > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, 2021-01-12 at 15:36 -0600, Bjorn Helgaas wrote: > Note subject line tips at https://lore.kernel.org/r/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com > > On Tue, Jan 12, 2021 at 03:27:39PM +0800, mingchuang.qiao@mediatek.com wrote: > > From: Mingchuang Qiao <mingchuang.qiao@mediatek.com> > > > > In pci bus scan flow, the LTR mechanism enable bit of DEVCTL2 register > > is configured in pci_configure_ltr(). If device and it's bridge both > > support LTR mechanism, LTR mechanism of device and it's bridge will > > be enabled in DEVCTL2 register. And the flag pci_dev->ltr_path will > > be set as 1. > > s/it's/its/ twice above. > It's == It is. > Its == belonging to 'it'. > Weird, I know, but that's English for you :) > > > For some pcie products, pcie link becomes down when device reset. And then > > the LTR mechanism enable bit of bridge will become 0 based on description > > in PCIE r4.0, sec 7.8.16. However, the pci_dev->ltr_path value of bridge > > is still 1. Remove and rescan flow could be triggered to recover after > > device reset. In the bus rescan flow, the LTR mechanism of device will be > > enabled in pci_configure_ltr() due to ltr_path of its bridge is still 1. > > s/pcie/PCIe/ twice above. > s/PCIE/PCIe/; also reference r5.0 instead of r4.0 if possible. > Sorry for the misspelling and inconvenience. Thanks for the correction and I will follow the suggestion in later patch. > This sounds like a general problem of most device config bits being > cleared by reset. Usually these are restored by pci_restore_state(). > Is that function missing a restore for PCI_EXP_DEVCTL2? I'd rather > have a general-purpose way of restoring all the config state than > little pieces scattered all over. > Actually it’s not PCI_EXP_DEVCTL2 restore missing issue. The PCI_EXP_DEVCTL2 is correctly saved/restored during bridge runtime suspend/resume. It’s the issue that ltr_path value of the bridge mismatches the actual value in bridge’s PCI_EXP_DEVCTL2. Here is the scenario for the issue: 1.PCI bus scan done -For both PCIe Device and bridge: -"LTR Mechanism Enable" bit in PCI_EXP_DEVCTL2 is 1 - ltr_path value is 1 2.Device resets and PCIe link is down, then "LTR Mechanism Enable" bit of bridge changes to 0 according to PCIe r5.0, sec 7.5.3.16. -The ltr_path value of bridge is still 1 but the "LTR Mechanism Enable" bit within PCI_EXP_DEVCTL2 changed to 0. 3.Trigger device removal and bridge enters runtime suspend flow. -"LTR Mechanism Enable" bit of bridge is 0 and saved by pci_save_state(). 4.Trigger bus rescan and bridge enters runtime resume flow. -"LTR Mechanism Enable" bit of bridge is restored by pci_restore_state() and the value is 0. 5.Scan device and configure device's LTR in pci_configure_ltr(). -"LTR Mechanism Enable" bit of device is configured as 1 due to bridge's ltr_path value is 1. 6.The "LTR Mechanism Enable" bit of device and bridge is different now. -When device sends LTR Message, bridge will treat the Message as Unsupported Request according to PCIe r5.0, sec 6.18. This patch is used to make bridge's ltr_path value match "LTR Mechanism Enable" bit within DEVCTL2 register and avoid the Unsupported Request. > > When device's LTR mechanism is enabled, device will send LTR message to > > bridge. Bridge receives the device's LTR message and found bridge's LTR > > mechanism is disabled. Then the bridge will generate unsupported request > > and other error handling flow will be triggered such as AER Non-Fatal > > error handling. > > > > This patch is used to avoid this unsupported request and make the bridge's > > ltr_path value is aligned with DEVCTL2 register value. Check bridge > > register value if aligned with ltr_path in pci_configure_ltr(). If > > register value is disable and the ltr_path is 1, we need configure > > the register value as enable. > > > > Signed-off-by: Mingchuang Qiao <mingchuang.qiao@mediatek.com> > > --- > > drivers/pci/probe.c | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 953f15abc850..49355cf4af54 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -2132,9 +2132,21 @@ static void pci_configure_ltr(struct pci_dev *dev) > > * Complex and all intermediate Switches indicate support for LTR. > > * PCIe r4.0, sec 6.18. > > */ > > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || > > - ((bridge = pci_upstream_bridge(dev)) && > > - bridge->ltr_path)) { > > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) { > > + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, > > + PCI_EXP_DEVCTL2_LTR_EN); > > + dev->ltr_path = 1; > > + return; > > + } > > + > > + bridge = pci_upstream_bridge(dev); > > + if (bridge && bridge->ltr_path) { > > + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl); > > + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) { > > + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2, > > + PCI_EXP_DEVCTL2_LTR_EN); > > + } > > + > > pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, > > PCI_EXP_DEVCTL2_LTR_EN); > > dev->ltr_path = 1; > > -- > > 2.18.0 > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 953f15abc850..49355cf4af54 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2132,9 +2132,21 @@ static void pci_configure_ltr(struct pci_dev *dev) * Complex and all intermediate Switches indicate support for LTR. * PCIe r4.0, sec 6.18. */ - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || - ((bridge = pci_upstream_bridge(dev)) && - bridge->ltr_path)) { + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) { + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, + PCI_EXP_DEVCTL2_LTR_EN); + dev->ltr_path = 1; + return; + } + + bridge = pci_upstream_bridge(dev); + if (bridge && bridge->ltr_path) { + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl); + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) { + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2, + PCI_EXP_DEVCTL2_LTR_EN); + } + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, PCI_EXP_DEVCTL2_LTR_EN); dev->ltr_path = 1;