lkml.org 
[lkml]   [2021]   [Jan]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] pci: avoid unsync of LTR mechanism configuration
From
Date
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

\
 
 \ /
  Last update: 2021-01-18 03:57    [W:0.130 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site