Re: [PATCH v2] PCI: Re-enable downstream port LTR if it was previously enabled
From: Bjorn Helgaas <helgaas@kernel.org>
Date: 2021-01-21 22:32:58
Also in:
linux-mediatek, linux-pci, lkml
[+cc Alex and Mingchuang et al from https://lore.kernel.org/r/20210112072739.31624-1-mingchuang.qiao@mediatek.com (local)] On Tue, Jan 19, 2021 at 04:14:10PM +0300, Mika Westerberg wrote:
PCIe r5.0, sec 7.5.3.16 says that the downstream ports must reset the LTR enable bit if the link goes down (port goes DL_Down status). Now, if we had LTR previously enabled and the PCIe endpoint gets hot-removed and then hot-added back the ->ltr_path of the downstream port is still set but the port now does not have the LTR enable bit set anymore. For this reason check if the bridge upstream had LTR enabled previously and re-enable it before enabling LTR for the endpoint. Reported-by: Utkarsh H Patel <redacted> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
I think this and Mingchuang's patch, which is essentially identical, are right and solves the problem for hot-remove/hot-add. In that scenario we call pci_configure_ltr() on the hot-added device, and with this patch, we'll re-enable LTR on the bridge leading to the new device before enabling LTR on the new device itself. But don't we have a similar problem if we simply do a Fundamental Reset on a device? I think the reset path will restore the device's state, including PCI_EXP_DEVCTL2, but it doesn't do anything with the upstream bridge, does it? So if a bridge and a device below it both have LTR enabled, can't we have the following: - bridge LTR enabled - device LTR enabled - reset device, e.g., via Secondary Bus Reset - link goes down, bridge disables LTR - link comes back up, LTR disabled in both bridge and device - restore device state, including LTR enable - device sends LTR message - bridge reports Unsupported Request
quoted hunk
--- Previous version can be found here: https://lore.kernel.org/linux-pci/20210114134724.79511-1-mika.westerberg@linux.intel.com/ (local) Changes from the previous version: * Corrected typos in the commit message * No need to call pcie_downstream_port() drivers/pci/probe.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 0eb68b47354f..a4a8c0305fb9 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c@@ -2153,7 +2153,7 @@ static void pci_configure_ltr(struct pci_dev *dev) { #ifdef CONFIG_PCIEASPM struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); - struct pci_dev *bridge; + struct pci_dev *bridge = NULL; u32 cap, ctl; if (!pci_is_pcie(dev))@@ -2191,6 +2191,21 @@ static void pci_configure_ltr(struct pci_dev *dev) if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || ((bridge = pci_upstream_bridge(dev)) && bridge->ltr_path)) { + /* + * Downstream ports reset the LTR enable bit when the + * link goes down (e.g on hot-remove) so re-enable the + * bit here if not set anymore. + * PCIe r5.0, sec 7.5.3.16. + */ + if (bridge) { + 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); + } + } + pci_dbg(dev, "enabling LTR\n"); pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, PCI_EXP_DEVCTL2_LTR_EN); dev->ltr_path = 1;-- 2.29.2
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel