ACK: [SRU][I/J/OEM-5.13/OEM-5.14][PATCH 1/1] PCI: Re-enable Downstream Port LTR after reset or hotplug

[Kleber Souza]

On 13.12.21 12:52, Aaron Ma wrote:
> From: Mingchuang Qiao <mingchuang.qiao at mediatek.com>
>
> BugLink: https://bugs.launchpad.net/bugs/1954646
>
> Per PCIe r5.0, sec 7.5.3.16, Downstream Ports must disable LTR if the link
> goes down (the Port goes DL_Down status).  This is a problem because the
> Downstream Port's dev->ltr_path is still set, so we think LTR is still
> enabled, and we enable LTR in the Endpoint.  When it sends LTR messages,
> they cause Unsupported Request errors at the Downstream Port.
>
> This happens in the reset path, where we may enable LTR in
> pci_restore_pcie_state() even though the Downstream Port disabled LTR
> because the reset caused a link down event.
>
> It also happens in the hot-remove and hot-add path, where we may enable LTR
> in pci_configure_ltr() even though the Downstream Port disabled LTR when
> the hot-remove took the link down.
>
> In these two scenarios, check the upstream bridge and restore its LTR
> enable if appropriate.
>
> The Unsupported Request may be logged by AER as follows:
>
>    pcieport 0000:00:1d.0: AER: Uncorrected (Non-Fatal) error received: id=00e8
>    pcieport 0000:00:1d.0: PCIe Bus Error: severity=Uncorrected (Non-Fatal), type=Transaction Layer, id=00e8(Requester ID)
>    pcieport 0000:00:1d.0:   device [8086:9d18] error status/mask=00100000/00010000
>    pcieport 0000:00:1d.0:    [20] Unsupported Request    (First)
>
> In addition, if LTR is not configured correctly, the link cannot enter the
> L1.2 state, which prevents some machines from entering the S0ix low power
> state.
>
> [bhelgaas: commit log]
> Link: https://lore.kernel.org/r/20211012075614.54576-1-mingchuang.qiao@mediatek.com
> Reported-by: Utkarsh H Patel <utkarsh.h.patel at intel.com>
> Signed-off-by: Mingchuang Qiao <mingchuang.qiao at mediatek.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas at google.com>
> Reviewed-by: Mika Westerberg <mika.westerberg at linux.intel.com>
> (cherry picked from commit e1b0d0bb2032d18c7718168e670d8d3f31e552d7)
> Signed-off-by: Aaron Ma <aaron.ma at canonical.com>
I guess there's a bit of a risk with these PCI changes but as of now
there's no upstream follow-up fix, and if there is any major issue
with the code we'll likely hit it during tests.


Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>

Thanks

> ---
>   drivers/pci/pci.c   | 25 +++++++++++++++++++++++++
>   drivers/pci/pci.h   |  1 +
>   drivers/pci/probe.c | 18 +++++++++++++++---
>   3 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a101faf3e88a..9dbf92a7f5aa 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1477,6 +1477,24 @@ static int pci_save_pcie_state(struct pci_dev *dev)
>   	return 0;
>   }
>   
> +void pci_bridge_reconfigure_ltr(struct pci_dev *dev)
> +{
> +#ifdef CONFIG_PCIEASPM
> +	struct pci_dev *bridge;
> +	u32 ctl;
> +
> +	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)) {
> +			pci_dbg(bridge, "re-enabling LTR\n");
> +			pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
> +						 PCI_EXP_DEVCTL2_LTR_EN);
> +		}
> +	}
> +#endif
> +}
> +
>   static void pci_restore_pcie_state(struct pci_dev *dev)
>   {
>   	int i = 0;
> @@ -1487,6 +1505,13 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>   	if (!save_state)
>   		return;
>   
> +	/*
> +	 * Downstream ports reset the LTR enable bit when link goes down.
> +	 * Check and re-configure the bit here before restoring device.
> +	 * PCIe r5.0, sec 7.5.3.16.
> +	 */
> +	pci_bridge_reconfigure_ltr(dev);
> +
>   	cap = (u16 *)&save_state->cap.data[0];
>   	pcie_capability_write_word(dev, PCI_EXP_DEVCTL, cap[i++]);
>   	pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 1cce56c2aea0..bc7f3a10ff60 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -125,6 +125,7 @@ void pci_msix_init(struct pci_dev *dev);
>   bool pci_bridge_d3_possible(struct pci_dev *dev);
>   void pci_bridge_d3_update(struct pci_dev *dev);
>   void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev);
> +void pci_bridge_reconfigure_ltr(struct pci_dev *dev);
>   
>   static inline void pci_wakeup_event(struct pci_dev *dev)
>   {
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index e322907d1f3c..cb70d2605e97 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2210,9 +2210,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;
> +	}
> +
> +	/*
> +	 * If we're configuring a hot-added device, LTR was likely
> +	 * disabled in the upstream bridge, so re-enable it before enabling
> +	 * it in the new device.
> +	 */
> +	bridge = pci_upstream_bridge(dev);
> +	if (bridge && bridge->ltr_path) {
> +		pci_bridge_reconfigure_ltr(dev);
>   		pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
>   					 PCI_EXP_DEVCTL2_LTR_EN);
>   		dev->ltr_path = 1;