The ASPM L1.2 substate depends on LTR information. Per the PCI
Firmware spec, the OS is supposed to negotiate with the platform for
control of the LTR feature, but previously we didn't do that.
In addition, we must not enable LTR in an endpoint unless the Root
Complex and all intermediate switches also support LTR. We already
took care of that in pci_configure_ltr(), but we didn't ensure that
LTR was enabled before allowing ASPM L1.2 to be enabled.
These patches fix both of these issues. Or rather, they *should* fix
them. I don't have hardware to test them, so any help with testing
would be appreciated.
I think the most likely issue would be a platform where the hardware
supports LTR and the ASPM L1.2 substate, but the BIOS doesn't support
LTR in _OSC. In that case, we previously could have enabled ASPM L1.2
(though it probably wouldn't work correctly), and after these patches,
we should not enable ASPM L1.2.
You can look for issues by comparing dmesg and "lspci -vv" output
before and after these patches.
It would also be interesting to collect an acpidump from platforms
that support LTR, even if there's no endpoint that supports ASPM L1.2.
The acpidump should show that _OSC supports LTR.
I included some NVMe folks because these were motivated by Srinath's
recent report of LTR and ASPM issues with a Samsung NVMe SSD
Controller SM961/PM961 device, so this is sort of FYI in case you see
similar issues or are in a position to test these.
---
Bjorn Helgaas (2):
PCI/ASPM: Disable ASPM L1.2 Substate if we don't have LTR
PCI/ACPI: Request LTR control from platform before using it
drivers/acpi/pci_root.c | 7 +++++++
drivers/pci/pcie/aspm.c | 9 +++++++++
drivers/pci/probe.c | 5 +++++
include/linux/acpi.h | 3 ++-
include/linux/pci.h | 1 +
5 files changed, 24 insertions(+), 1 deletion(-)
From: Bjorn Helgaas <[email protected]>
Per the PCI Firmware spec r3.2, sec 4.5, an ACPI-based OS should use _OSC
to request control of Latency Tolerance Reporting (LTR) before using it.
Request control of LTR, and if the platform does not grant control, don't
use it.
N.B. If the hardware supports LTR and the ASPM L1.2 substate but the BIOS
doesn't support LTR in _OSC, we previously would enable ASPM L1.2. This
patch will prevent us from enabling ASPM L1.2 in that case. It does not
prevent us from enabling PCI-PM L1.2, since that doesn't depend on LTR.
See PCIe r40, sec 5.5.1, for the L1 PM substate entry conditions.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/acpi/pci_root.c | 7 +++++++
drivers/pci/probe.c | 5 +++++
include/linux/acpi.h | 3 ++-
include/linux/pci.h | 1 +
4 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 0da18bde6a16..f32d767e8e3b 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -153,6 +153,7 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
{ OSC_PCI_EXPRESS_PME_CONTROL, "PME" },
{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
+ { OSC_PCI_EXPRESS_LTR_CONTROL, "LTR" },
};
static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
@@ -475,6 +476,10 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
| OSC_PCI_EXPRESS_PME_CONTROL;
+#ifdef CONFIG_PCIEASPM
+ control |= OSC_PCI_EXPRESS_LTR_CONTROL;
+#endif
+
if (pci_aer_available()) {
if (aer_acpi_firmware_first())
dev_info(&device->dev,
@@ -905,6 +910,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
host_bridge->native_aer = 0;
if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
host_bridge->native_pme = 0;
+ if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
+ host_bridge->native_ltr = 0;
pci_scan_child_bus(bus);
pci_set_host_bridge_release(host_bridge, acpi_pci_root_release_info,
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac91b6fd0bcd..cc1688d75664 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -554,6 +554,7 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
bridge->native_aer = 1;
bridge->native_hotplug = 1;
bridge->native_pme = 1;
+ bridge->native_ltr = 1;
return bridge;
}
@@ -1954,9 +1955,13 @@ static void pci_configure_relaxed_ordering(struct pci_dev *dev)
static void pci_configure_ltr(struct pci_dev *dev)
{
#ifdef CONFIG_PCIEASPM
+ struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
u32 cap;
struct pci_dev *bridge;
+ if (!host->native_ltr)
+ return;
+
if (!pci_is_pcie(dev))
return;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 15bfb15c2fa5..49f63c67a9d1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -506,7 +506,8 @@ extern bool osc_pc_lpi_support_confirmed;
#define OSC_PCI_EXPRESS_PME_CONTROL 0x00000004
#define OSC_PCI_EXPRESS_AER_CONTROL 0x00000008
#define OSC_PCI_EXPRESS_CAPABILITY_CONTROL 0x00000010
-#define OSC_PCI_CONTROL_MASKS 0x0000001f
+#define OSC_PCI_EXPRESS_LTR_CONTROL 0x00000020
+#define OSC_PCI_CONTROL_MASKS 0x0000003f
#define ACPI_GSB_ACCESS_ATTRIB_QUICK 0x00000002
#define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV 0x00000004
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 73178a2fcee0..d0149c01996d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -473,6 +473,7 @@ struct pci_host_bridge {
unsigned int native_aer:1; /* OS may use PCIe AER */
unsigned int native_hotplug:1; /* OS may use PCIe hotplug */
unsigned int native_pme:1; /* OS may use PCIe PME */
+ unsigned int native_ltr:1; /* OS may use PCIe LTR */
/* Resource alignment requirements */
resource_size_t (*align_resource)(struct pci_dev *dev,
const struct resource *res,
From: Bjorn Helgaas <[email protected]>
When in the ASPM L1.0 state (but not the PCI-PM L1.0 state), the most
recent LTR value and the LTR_L1.2_THRESHOLD determines whether the link
enters the L1.2 substate.
If we don't have LTR enabled, prevent the use of ASPM L1.2.
PCI-PM L1.2 may still be used because it doesn't depend on
LTR_L1.2_THRESHOLD (see PCIe r4.0, sec 5.5.1).
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/pcie/aspm.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index f76eb7704f64..c687c817b47d 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -400,6 +400,15 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
info->l1ss_cap = 0;
return;
}
+
+ /*
+ * If we don't have LTR for the entire path from the Root Complex
+ * to this device, we can't use ASPM L1.2 because it relies on the
+ * LTR_L1.2_THRESHOLD. See PCIe r4.0, secs 5.5.4, 6.18.
+ */
+ if (!pdev->ltr_path)
+ info->l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;
+
pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL1,
&info->l1ss_ctl1);
pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL2,
Hi Bjorn,
I have verified this patch, it works fine for me.
Thanks & Regards,
Srinath.
On Fri, Apr 20, 2018 at 2:35 AM, Bjorn Helgaas <[email protected]> wrote:
> From: Bjorn Helgaas <[email protected]>
>
> When in the ASPM L1.0 state (but not the PCI-PM L1.0 state), the most
> recent LTR value and the LTR_L1.2_THRESHOLD determines whether the link
> enters the L1.2 substate.
>
> If we don't have LTR enabled, prevent the use of ASPM L1.2.
>
> PCI-PM L1.2 may still be used because it doesn't depend on
> LTR_L1.2_THRESHOLD (see PCIe r4.0, sec 5.5.1).
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/pci/pcie/aspm.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index f76eb7704f64..c687c817b47d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -400,6 +400,15 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
> info->l1ss_cap = 0;
> return;
> }
> +
> + /*
> + * If we don't have LTR for the entire path from the Root Complex
> + * to this device, we can't use ASPM L1.2 because it relies on the
> + * LTR_L1.2_THRESHOLD. See PCIe r4.0, secs 5.5.4, 6.18.
> + */
> + if (!pdev->ltr_path)
> + info->l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;
> +
> pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL1,
> &info->l1ss_ctl1);
> pci_read_config_dword(pdev, info->l1ss_cap_ptr + PCI_L1SS_CTL2,
>
On Thursday, April 19, 2018 11:05:19 PM CEST Bjorn Helgaas wrote:
> The ASPM L1.2 substate depends on LTR information. Per the PCI
> Firmware spec, the OS is supposed to negotiate with the platform for
> control of the LTR feature, but previously we didn't do that.
>
> In addition, we must not enable LTR in an endpoint unless the Root
> Complex and all intermediate switches also support LTR. We already
> took care of that in pci_configure_ltr(), but we didn't ensure that
> LTR was enabled before allowing ASPM L1.2 to be enabled.
>
> These patches fix both of these issues. Or rather, they *should* fix
> them. I don't have hardware to test them, so any help with testing
> would be appreciated.
>
> I think the most likely issue would be a platform where the hardware
> supports LTR and the ASPM L1.2 substate, but the BIOS doesn't support
> LTR in _OSC. In that case, we previously could have enabled ASPM L1.2
> (though it probably wouldn't work correctly), and after these patches,
> we should not enable ASPM L1.2.
>
> You can look for issues by comparing dmesg and "lspci -vv" output
> before and after these patches.
>
> It would also be interesting to collect an acpidump from platforms
> that support LTR, even if there's no endpoint that supports ASPM L1.2.
> The acpidump should show that _OSC supports LTR.
>
> I included some NVMe folks because these were motivated by Srinath's
> recent report of LTR and ASPM issues with a Samsung NVMe SSD
> Controller SM961/PM961 device, so this is sort of FYI in case you see
> similar issues or are in a position to test these.
>
> ---
>
> Bjorn Helgaas (2):
> PCI/ASPM: Disable ASPM L1.2 Substate if we don't have LTR
> PCI/ACPI: Request LTR control from platform before using it
For both:
Reviewed-by: Rafael J. Wysocki <[email protected]>
On Thu, Apr 19, 2018 at 04:05:19PM -0500, Bjorn Helgaas wrote:
> The ASPM L1.2 substate depends on LTR information. Per the PCI
> Firmware spec, the OS is supposed to negotiate with the platform for
> control of the LTR feature, but previously we didn't do that.
>
> In addition, we must not enable LTR in an endpoint unless the Root
> Complex and all intermediate switches also support LTR. We already
> took care of that in pci_configure_ltr(), but we didn't ensure that
> LTR was enabled before allowing ASPM L1.2 to be enabled.
>
> These patches fix both of these issues. Or rather, they *should* fix
> them. I don't have hardware to test them, so any help with testing
> would be appreciated.
>
> I think the most likely issue would be a platform where the hardware
> supports LTR and the ASPM L1.2 substate, but the BIOS doesn't support
> LTR in _OSC. In that case, we previously could have enabled ASPM L1.2
> (though it probably wouldn't work correctly), and after these patches,
> we should not enable ASPM L1.2.
>
> You can look for issues by comparing dmesg and "lspci -vv" output
> before and after these patches.
>
> It would also be interesting to collect an acpidump from platforms
> that support LTR, even if there's no endpoint that supports ASPM L1.2.
> The acpidump should show that _OSC supports LTR.
>
> I included some NVMe folks because these were motivated by Srinath's
> recent report of LTR and ASPM issues with a Samsung NVMe SSD
> Controller SM961/PM961 device, so this is sort of FYI in case you see
> similar issues or are in a position to test these.
>
> ---
>
> Bjorn Helgaas (2):
> PCI/ASPM: Disable ASPM L1.2 Substate if we don't have LTR
> PCI/ACPI: Request LTR control from platform before using it
>
>
> drivers/acpi/pci_root.c | 7 +++++++
> drivers/pci/pcie/aspm.c | 9 +++++++++
> drivers/pci/probe.c | 5 +++++
> include/linux/acpi.h | 3 ++-
> include/linux/pci.h | 1 +
> 5 files changed, 24 insertions(+), 1 deletion(-)
Applied to pci/aspm for v4.18 with Rafael's reviewed-by and Srinath's
tested-by (on first patch).