Re: [PATCH] pci: avoid unsync of LTR mechanism configuration

[Mingchuang Qiao <mingchuang.qiao@mediatek.com>]

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