[PATCH 0/3] PCI/ASPM: Fix L1SS issues

[PATCH 0/3] PCI/ASPM: Fix L1SS issues

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This is really late, but I think we have two significant issues with L1SS:

  1) pcie_aspm_cap_init() reads from the L1SS capability even when it
  doesn't exist, so it reads PCI_COMMAND and PCI_STATUS instead and treats
  those as an L1SS Capability value.

  2) encode_l12_threshold() encodes LTR_L1.2_THRESHOLD as smaller than
  requested, so ports may enter L1.2 when they should not.

These patches are intended to fix both issues.

Bjorn Helgaas (3):
  PCI/ASPM: Factor out L1 PM Substates configuration
  PCI/ASPM: Ignore L1 PM Substates if device lacks capability
  PCI/ASPM: Correct LTR_L1.2_THRESHOLD computation

 drivers/pci/pcie/aspm.c | 155 +++++++++++++++++++++++-----------------
 1 file changed, 90 insertions(+), 65 deletions(-)

-- 
2.25.1

[PATCH 1/3] PCI/ASPM: Factor out L1 PM Substates configuration

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Move L1 PM Substates configuration from pcie_aspm_cap_init() to a new
aspm_l1ss_init() function.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 103 +++++++++++++++++++++-------------------
 1 file changed, 55 insertions(+), 48 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 016d222b07c7..4535228e4a64 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -554,13 +554,65 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	aspm_program_l1ss(child, cctl1, ctl2);
 }
 
+static void aspm_l1ss_init(struct pcie_link_state *link)
+{
+	struct pci_dev *child = link->downstream, *parent = link->pdev;
+	u32 parent_l1ss_cap, child_l1ss_cap;
+	u32 parent_l1ss_ctl1 = 0, child_l1ss_ctl1 = 0;
+
+	/* Setup L1 substate */
+	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CAP,
+			      &parent_l1ss_cap);
+	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CAP,
+			      &child_l1ss_cap);
+
+	if (!(parent_l1ss_cap & PCI_L1SS_CAP_L1_PM_SS))
+		parent_l1ss_cap = 0;
+	if (!(child_l1ss_cap & PCI_L1SS_CAP_L1_PM_SS))
+		child_l1ss_cap = 0;
+
+	/*
+	 * 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 (!child->ltr_path)
+		child_l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;
+
+	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
+		link->aspm_support |= ASPM_STATE_L1_1;
+	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
+		link->aspm_support |= ASPM_STATE_L1_2;
+	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_1)
+		link->aspm_support |= ASPM_STATE_L1_1_PCIPM;
+	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
+		link->aspm_support |= ASPM_STATE_L1_2_PCIPM;
+
+	if (parent_l1ss_cap)
+		pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
+				      &parent_l1ss_ctl1);
+	if (child_l1ss_cap)
+		pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
+				      &child_l1ss_ctl1);
+
+	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
+		link->aspm_enabled |= ASPM_STATE_L1_1;
+	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2)
+		link->aspm_enabled |= ASPM_STATE_L1_2;
+	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1)
+		link->aspm_enabled |= ASPM_STATE_L1_1_PCIPM;
+	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
+		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
+
+	if (link->aspm_support & ASPM_STATE_L1SS)
+		aspm_calc_l1ss_info(link, parent_l1ss_cap, child_l1ss_cap);
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
 	u32 parent_lnkcap, child_lnkcap;
 	u16 parent_lnkctl, child_lnkctl;
-	u32 parent_l1ss_cap, child_l1ss_cap;
-	u32 parent_l1ss_ctl1 = 0, child_l1ss_ctl1 = 0;
 	struct pci_bus *linkbus = parent->subordinate;
 
 	if (blacklist) {
@@ -615,52 +667,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	if (parent_lnkctl & child_lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
 		link->aspm_enabled |= ASPM_STATE_L1;
 
-	/* Setup L1 substate */
-	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CAP,
-			      &parent_l1ss_cap);
-	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CAP,
-			      &child_l1ss_cap);
-
-	if (!(parent_l1ss_cap & PCI_L1SS_CAP_L1_PM_SS))
-		parent_l1ss_cap = 0;
-	if (!(child_l1ss_cap & PCI_L1SS_CAP_L1_PM_SS))
-		child_l1ss_cap = 0;
-
-	/*
-	 * 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 (!child->ltr_path)
-		child_l1ss_cap &= ~PCI_L1SS_CAP_ASPM_L1_2;
-
-	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_ASPM_L1_1)
-		link->aspm_support |= ASPM_STATE_L1_1;
-	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_ASPM_L1_2)
-		link->aspm_support |= ASPM_STATE_L1_2;
-	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_1)
-		link->aspm_support |= ASPM_STATE_L1_1_PCIPM;
-	if (parent_l1ss_cap & child_l1ss_cap & PCI_L1SS_CAP_PCIPM_L1_2)
-		link->aspm_support |= ASPM_STATE_L1_2_PCIPM;
-
-	if (parent_l1ss_cap)
-		pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
-				      &parent_l1ss_ctl1);
-	if (child_l1ss_cap)
-		pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
-				      &child_l1ss_ctl1);
-
-	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_1)
-		link->aspm_enabled |= ASPM_STATE_L1_1;
-	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_ASPM_L1_2)
-		link->aspm_enabled |= ASPM_STATE_L1_2;
-	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_1)
-		link->aspm_enabled |= ASPM_STATE_L1_1_PCIPM;
-	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
-		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
-
-	if (link->aspm_support & ASPM_STATE_L1SS)
-		aspm_calc_l1ss_info(link, parent_l1ss_cap, child_l1ss_cap);
+	aspm_l1ss_init(link);
 
 	/* Save default state */
 	link->aspm_default = link->aspm_enabled;
-- 
2.25.1

[PATCH 2/3] PCI/ASPM: Ignore L1 PM Substates if device lacks capability

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

187f91db8237 ("PCI/ASPM: Remove struct aspm_register_info.l1ss_cap")
inadvertently removed a check for existence of the L1 PM Substates (L1SS)
Capability before reading it.

If there is no L1SS Capability, this means we mistakenly read PCI_COMMAND
and PCI_STATUS (config address 0x04) and interpret that as the PCI_L1SS_CAP
register, so we may incorrectly configure L1SS.

Make sure the L1SS Capability exists before trying to read it.

Fixes: 187f91db8237 ("PCI/ASPM: Remove struct aspm_register_info.l1ss_cap")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 4535228e4a64..f12d117f44e0 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -560,6 +560,9 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
 	u32 parent_l1ss_cap, child_l1ss_cap;
 	u32 parent_l1ss_ctl1 = 0, child_l1ss_ctl1 = 0;
 
+	if (!parent->l1ss || !child->l1ss)
+		return;
+
 	/* Setup L1 substate */
 	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CAP,
 			      &parent_l1ss_cap);
-- 
2.25.1

Re: [PATCH 2/3] PCI/ASPM: Ignore L1 PM Substates if device lacks capability

[Sathyanarayanan Kuppuswamy]

On 10/4/22 7:58 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> 187f91db8237 ("PCI/ASPM: Remove struct aspm_register_info.l1ss_cap")
> inadvertently removed a check for existence of the L1 PM Substates (L1SS)
> Capability before reading it.
> 
> If there is no L1SS Capability, this means we mistakenly read PCI_COMMAND
> and PCI_STATUS (config address 0x04) and interpret that as the PCI_L1SS_CAP
> register, so we may incorrectly configure L1SS.
> 
> Make sure the L1SS Capability exists before trying to read it.
> 
> Fixes: 187f91db8237 ("PCI/ASPM: Remove struct aspm_register_info.l1ss_cap")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/aspm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 4535228e4a64..f12d117f44e0 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -560,6 +560,9 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
>  	u32 parent_l1ss_cap, child_l1ss_cap;
>  	u32 parent_l1ss_ctl1 = 0, child_l1ss_ctl1 = 0;
>  
> +	if (!parent->l1ss || !child->l1ss)
> +		return;

I have noticed that l1ss state is initialized in pci_configure_ltr(). I am
wondering whether it is the right place? Are L1SS and LTR related?


> +
>  	/* Setup L1 substate */
>  	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CAP,
>  			      &parent_l1ss_cap);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 2/3] PCI/ASPM: Ignore L1 PM Substates if device lacks capability

[Bjorn Helgaas]

On Tue, Oct 04, 2022 at 08:26:53PM -0700, Sathyanarayanan Kuppuswamy wrote:
> On 10/4/22 7:58 PM, Bjorn Helgaas wrote:
> > 187f91db8237 ("PCI/ASPM: Remove struct aspm_register_info.l1ss_cap")
> > inadvertently removed a check for existence of the L1 PM Substates (L1SS)
> > Capability before reading it.
> > 
> > If there is no L1SS Capability, this means we mistakenly read PCI_COMMAND
> > and PCI_STATUS (config address 0x04) and interpret that as the PCI_L1SS_CAP
> > register, so we may incorrectly configure L1SS.
> > 
> > Make sure the L1SS Capability exists before trying to read it.
> > 
> > Fixes: 187f91db8237 ("PCI/ASPM: Remove struct aspm_register_info.l1ss_cap")
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/pcie/aspm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 4535228e4a64..f12d117f44e0 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -560,6 +560,9 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
> >  	u32 parent_l1ss_cap, child_l1ss_cap;
> >  	u32 parent_l1ss_ctl1 = 0, child_l1ss_ctl1 = 0;
> >  
> > +	if (!parent->l1ss || !child->l1ss)
> > +		return;
> 
> I have noticed that l1ss state is initialized in pci_configure_ltr(). I am
> wondering whether it is the right place? Are L1SS and LTR related?

L1SS and LTR are definitely related -- LTR supplies information
that's needed for L1.2.

I'm not sure why pci_configure_ltr() is in probe.c and
pci_bridge_reconfigure_ltr() is in pci.c; maybe it would make
sense to put them both in aspm.c.  

Bjorn

[PATCH 3/3] PCI/ASPM: Correct LTR_L1.2_THRESHOLD computation

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

80d7d7a904fa ("PCI/ASPM: Calculate LTR_L1.2_THRESHOLD from device
characteristics") replaced a fixed value (163840ns) with one computed from
T_POWER_OFF, Common_Mode_Restore_Time, etc., but it encoded the
LTR_L1.2_THRESHOLD value incorrectly.

This is especially a problem for small thresholds, e.g., 63ns fell into the
"threshold_ns < 1024" case and was encoded as 32ns:

  LTR_L1.2_THRESHOLD_Scale = 1 (multiplier is 32ns)
  LTR_L1.2_THRESHOLD_Value = 63 >> 5 = 1
  LTR_L1.2_THRESHOLD       = multiplier * value = 32ns * 1 = 32ns

Correct the algorithm to encode all times of 1023ns (0x3ff) or smaller
exactly and larger times conservatively (the encoded threshold is never
smaller than was requested).  This reduces the chance of entering L1.2
when the device can't tolerate the exit latency.

Fixes: 80d7d7a904fa ("PCI/ASPM: Calculate LTR_L1.2_THRESHOLD from device characteristics")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 49 +++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index f12d117f44e0..53a1fa306e1e 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/math.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/pci.h>
@@ -350,29 +351,43 @@ static u32 calc_l1ss_pwron(struct pci_dev *pdev, u32 scale, u32 val)
 	return 0;
 }
 
+/*
+ * Encode an LTR_L1.2_THRESHOLD value for the L1 PM Substates Control 1
+ * register.  Ports enter L1.2 when the most recent LTR value is greater
+ * than or equal to LTR_L1.2_THRESHOLD, so we round up to make sure we
+ * don't enter L1.2 too aggressively.
+ *
+ * See PCIe r6.0, sec 5.5.1, 6.18, 7.8.3.3.
+ */
 static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 {
-	u32 threshold_ns = threshold_us * 1000;
+	u64 threshold_ns = (u64) threshold_us * 1000;
 
-	/* See PCIe r3.1, sec 7.33.3 and sec 6.18 */
-	if (threshold_ns < 32) {
-		*scale = 0;
+	/*
+	 * LTR_L1.2_THRESHOLD_Value ("value") is a 10-bit field with max
+	 * value of 0x3ff.
+	 */
+	if (threshold_ns <= 0x3ff * 1) {
+		*scale = 0;		/* Value times 1ns */
 		*value = threshold_ns;
-	} else if (threshold_ns < 1024) {
-		*scale = 1;
-		*value = threshold_ns >> 5;
-	} else if (threshold_ns < 32768) {
-		*scale = 2;
-		*value = threshold_ns >> 10;
-	} else if (threshold_ns < 1048576) {
-		*scale = 3;
-		*value = threshold_ns >> 15;
-	} else if (threshold_ns < 33554432) {
-		*scale = 4;
-		*value = threshold_ns >> 20;
+	} else if (threshold_ns <= 0x3ff * 32) {
+		*scale = 1;		/* Value times 32ns */
+		*value = roundup(threshold_ns, 32) / 32;
+	} else if (threshold_ns <= 0x3ff * 1024) {
+		*scale = 2;		/* Value times 1024ns */
+		*value = roundup(threshold_ns, 1024) / 1024;
+	} else if (threshold_ns <= 0x3ff * 32768) {
+		*scale = 3;		/* Value times 32768ns */
+		*value = roundup(threshold_ns, 32768) / 32768;
+	} else if (threshold_ns <= 0x3ff * 1048576) {
+		*scale = 4;		/* Value times 1048576ns */
+		*value = roundup(threshold_ns, 1048576) / 1048576;
+	} else if (threshold_ns <= 0x3ff * (u64) 33554432) {
+		*scale = 5;		/* Value times 33554432ns */
+		*value = roundup(threshold_ns, 33554432) / 33554432;
 	} else {
 		*scale = 5;
-		*value = threshold_ns >> 25;
+		*value = 0x3ff;		/* Max representable value */
 	}
 }
 
-- 
2.25.1

Re: [PATCH 0/3] PCI/ASPM: Fix L1SS issues

[Sathyanarayanan Kuppuswamy]

On 10/4/22 7:58 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This is really late, but I think we have two significant issues with L1SS:
> 
>   1) pcie_aspm_cap_init() reads from the L1SS capability even when it
>   doesn't exist, so it reads PCI_COMMAND and PCI_STATUS instead and treats
>   those as an L1SS Capability value.
> 
>   2) encode_l12_threshold() encodes LTR_L1.2_THRESHOLD as smaller than
>   requested, so ports may enter L1.2 when they should not.
> 
> These patches are intended to fix both issues.

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> 
> Bjorn Helgaas (3):
>   PCI/ASPM: Factor out L1 PM Substates configuration
>   PCI/ASPM: Ignore L1 PM Substates if device lacks capability
>   PCI/ASPM: Correct LTR_L1.2_THRESHOLD computation
> 
>  drivers/pci/pcie/aspm.c | 155 +++++++++++++++++++++++-----------------
>  1 file changed, 90 insertions(+), 65 deletions(-)
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 0/3] PCI/ASPM: Fix L1SS issues

[Bjorn Helgaas]

On Tue, Oct 04, 2022 at 08:28:07PM -0700, Sathyanarayanan Kuppuswamy wrote:
> On 10/4/22 7:58 PM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > This is really late, but I think we have two significant issues with L1SS:
> > 
> >   1) pcie_aspm_cap_init() reads from the L1SS capability even when it
> >   doesn't exist, so it reads PCI_COMMAND and PCI_STATUS instead and treats
> >   those as an L1SS Capability value.
> > 
> >   2) encode_l12_threshold() encodes LTR_L1.2_THRESHOLD as smaller than
> >   requested, so ports may enter L1.2 when they should not.
> > 
> > These patches are intended to fix both issues.
> 
> Looks good to me.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Thanks a lot for taking a look at these!  I put them on pci/aspm for
v6.1.

> > Bjorn Helgaas (3):
> >   PCI/ASPM: Factor out L1 PM Substates configuration
> >   PCI/ASPM: Ignore L1 PM Substates if device lacks capability
> >   PCI/ASPM: Correct LTR_L1.2_THRESHOLD computation
> > 
> >  drivers/pci/pcie/aspm.c | 155 +++++++++++++++++++++++-----------------
> >  1 file changed, 90 insertions(+), 65 deletions(-)
> > 
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer