[PATCH] pci: avoid unsync of LTR mechanism configuration

[PATCH] pci: avoid unsync of LTR mechanism configuration

[mingchuang.qiao]

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.

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.

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-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Bjorn Helgaas]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Mingchuang Qiao]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek