[PATCH v2 0/3] Let user enable ASPM and LTR via sysfs

[PATCH v2 0/3] Let user enable ASPM and LTR via sysfs

[Kai-Heng Feng]

v1:
https://lore.kernel.org/linux-pci/20201208082534.2460215-1-kai.heng.feng@canonical.com/

Older approach:
https://lore.kernel.org/linux-pci/20200930082455.25613-1-kai.heng.feng@canonical.com/

Kai-Heng Feng (3):
  PCI/ASPM: Store disabled ASPM states
  PCI/ASPM: Use capability to override ASPM via sysfs
  PCI/ASPM: Add LTR sysfs attributes

 drivers/pci/pcie/aspm.c | 74 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 71 insertions(+), 3 deletions(-)

-- 
2.32.0

[PATCH v2 1/3] PCI/ASPM: Store disabled ASPM states

[Kai-Heng Feng]

If we use sysfs to disable L1 ASPM, then enable one L1 ASPM substate
again, all other substates will also be enabled too:

link# grep . *
clkpm:1
l0s_aspm:1
l1_1_aspm:1
l1_1_pcipm:1
l1_2_aspm:1
l1_2_pcipm:1
l1_aspm:1

link# echo 0 > l1_aspm

link# grep . *
clkpm:1
l0s_aspm:1
l1_1_aspm:0
l1_1_pcipm:0
l1_2_aspm:0
l1_2_pcipm:0
l1_aspm:0

link# echo 1 > l1_2_aspm

link# grep . *
clkpm:1
l0s_aspm:1
l1_1_aspm:1
l1_1_pcipm:1
l1_2_aspm:1
l1_2_pcipm:1
l1_aspm:1

This is because disabled ASPM states weren't saved, so enable any of the
substate will also enable others.

So store the disabled ASPM states for consistency.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Drop redundant change.

 drivers/pci/pcie/aspm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 013a47f587ce..30b1bc899026 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -658,6 +658,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
 
+	link->aspm_disable = link->aspm_capable & ~link->aspm_default;
+
 	/* Get and check endpoint acceptable latencies */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
 		u32 reg32, encoding;
@@ -1231,6 +1233,9 @@ static ssize_t aspm_attr_store_common(struct device *dev,
 		if (state & ASPM_STATE_L1SS)
 			link->aspm_disable &= ~ASPM_STATE_L1;
 	} else {
+		if (state == ASPM_STATE_L1)
+			state |= ASPM_STATE_L1SS;
+
 		link->aspm_disable |= state;
 	}
 
-- 
2.32.0

[PATCH v2 2/3] PCI/ASPM: Use capability to override ASPM via sysfs

[Kai-Heng Feng]

If we are to use sysfs to change ASPM settings, we may want to override
the default ASPM policy.

So use ASPM capability, instead of default policy, to be able to use all
possible ASPM states.

Upon power management change, pcie_aspm_pm_state_change() always
re-apply the state from global ASPM policy, so also introduce a new flag
to make the sysfs change persistent.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - Add flag to make ASPM change persistent

 drivers/pci/pcie/aspm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 30b1bc899026..1560859ab056 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -60,6 +60,8 @@ struct pcie_link_state {
 	u32 aspm_default:7;		/* Default ASPM state by BIOS */
 	u32 aspm_disable:7;		/* Disabled ASPM state */
 
+	bool aspm_ignore_policy;	/* Ignore ASPM policy after sysfs overwrite */
+
 	/* Clock PM state */
 	u32 clkpm_capable:1;		/* Clock PM capable? */
 	u32 clkpm_enabled:1;		/* Current Clock PM state */
@@ -796,7 +798,8 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
 static void pcie_config_aspm_path(struct pcie_link_state *link)
 {
 	while (link) {
-		pcie_config_aspm_link(link, policy_to_aspm_state(link));
+		pcie_config_aspm_link(link, link->aspm_ignore_policy ?
+				      link->aspm_capable : policy_to_aspm_state(link));
 		link = link->parent;
 	}
 }
@@ -1238,8 +1241,8 @@ static ssize_t aspm_attr_store_common(struct device *dev,
 
 		link->aspm_disable |= state;
 	}
-
-	pcie_config_aspm_link(link, policy_to_aspm_state(link));
+	pcie_config_aspm_link(link, link->aspm_capable);
+	link->aspm_ignore_policy = true;
 
 	mutex_unlock(&aspm_lock);
 	up_read(&pci_bus_sem);
-- 
2.32.0

[PATCH v2 3/3] PCI/ASPM: Add LTR sysfs attributes

[Kai-Heng Feng]

Sometimes BIOS may not be able to program ASPM and LTR settings, for
instance, the NVMe devices behind Intel VMD bridges. For this case, both
ASPM and LTR have to be enabled to have significant power saving.

Since we still want POLICY_DEFAULT honor the default BIOS ASPM settings,
introduce LTR sysfs knobs so users can set max snoop and max nosnoop
latency manually or via udev rules.

[1] https://github.com/systemd/systemd/pull/17899/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209789
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
 - New patch.

 drivers/pci/pcie/aspm.c | 59 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 1560859ab056..f7dc62936445 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1299,6 +1299,59 @@ static ssize_t clkpm_store(struct device *dev,
 	return len;
 }
 
+static ssize_t ltr_attr_show_common(struct device *dev,
+			  struct device_attribute *attr, char *buf, u8 state)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	int ltr;
+	u16 val;
+
+	ltr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+	if (!ltr)
+		return -EINVAL;
+
+	pci_read_config_word(pdev, ltr + state, &val);
+
+	return sysfs_emit(buf, "0x%0x\n", val);
+}
+
+static ssize_t ltr_attr_store_common(struct device *dev,
+			   struct device_attribute *attr,
+			   const char *buf, size_t len, u8 state)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	int ltr;
+	u16 val;
+
+	ltr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+	if (!ltr)
+		return -EINVAL;
+
+	if (kstrtou16(buf, 16, &val) < 0)
+		return -EINVAL;
+
+	/* LatencyScale is not permitted to be 110 or 111 */
+	if ((val >> 10) > 5)
+		return -EINVAL;
+
+	pci_write_config_word(pdev, ltr + state, val);
+
+	return len;
+}
+
+#define LTR_ATTR(_f, _s)						\
+static ssize_t _f##_show(struct device *dev,				\
+			 struct device_attribute *attr, char *buf)	\
+{ return ltr_attr_show_common(dev, attr, buf, PCI_LTR_##_s); }		\
+									\
+static ssize_t _f##_store(struct device *dev,				\
+			  struct device_attribute *attr,		\
+			  const char *buf, size_t len)			\
+{ return ltr_attr_store_common(dev, attr, buf, len, PCI_LTR_##_s); }
+
+LTR_ATTR(ltr_max_snoop_lat, MAX_SNOOP_LAT);
+LTR_ATTR(ltr_max_nosnoop_lat, MAX_NOSNOOP_LAT);
+
 static DEVICE_ATTR_RW(clkpm);
 static DEVICE_ATTR_RW(l0s_aspm);
 static DEVICE_ATTR_RW(l1_aspm);
@@ -1306,6 +1359,8 @@ static DEVICE_ATTR_RW(l1_1_aspm);
 static DEVICE_ATTR_RW(l1_2_aspm);
 static DEVICE_ATTR_RW(l1_1_pcipm);
 static DEVICE_ATTR_RW(l1_2_pcipm);
+static DEVICE_ATTR_RW(ltr_max_snoop_lat);
+static DEVICE_ATTR_RW(ltr_max_nosnoop_lat);
 
 static struct attribute *aspm_ctrl_attrs[] = {
 	&dev_attr_clkpm.attr,
@@ -1315,6 +1370,8 @@ static struct attribute *aspm_ctrl_attrs[] = {
 	&dev_attr_l1_2_aspm.attr,
 	&dev_attr_l1_1_pcipm.attr,
 	&dev_attr_l1_2_pcipm.attr,
+	&dev_attr_ltr_max_snoop_lat.attr,
+	&dev_attr_ltr_max_nosnoop_lat.attr,
 	NULL
 };
 
@@ -1338,6 +1395,8 @@ static umode_t aspm_ctrl_attrs_are_visible(struct kobject *kobj,
 
 	if (n == 0)
 		return link->clkpm_capable ? a->mode : 0;
+	else if (n == 7 || n == 8)
+		return pdev->ltr_path ? a->mode : 0;
 
 	return link->aspm_capable & aspm_state_map[n - 1] ? a->mode : 0;
 }
-- 
2.32.0

Re: [PATCH v2 3/3] PCI/ASPM: Add LTR sysfs attributes

[Bjorn Helgaas]

On Thu, Oct 21, 2021 at 11:51:59AM +0800, Kai-Heng Feng wrote:
> Sometimes BIOS may not be able to program ASPM and LTR settings, for
> instance, the NVMe devices behind Intel VMD bridges. For this case, both
> ASPM and LTR have to be enabled to have significant power saving.
> 
> Since we still want POLICY_DEFAULT honor the default BIOS ASPM settings,
> introduce LTR sysfs knobs so users can set max snoop and max nosnoop
> latency manually or via udev rules.

How is the user supposed to figure out what "max snoop" and "max
nosnoop" values to program?

If we add this, I'm afraid we'll have people programming random things
that seem to work but are not actually reliable.

> [1] https://github.com/systemd/systemd/pull/17899/
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209789
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2:
>  - New patch.
> 
>  drivers/pci/pcie/aspm.c | 59 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 1560859ab056..f7dc62936445 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1299,6 +1299,59 @@ static ssize_t clkpm_store(struct device *dev,
>  	return len;
>  }
>  
> +static ssize_t ltr_attr_show_common(struct device *dev,
> +			  struct device_attribute *attr, char *buf, u8 state)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int ltr;
> +	u16 val;
> +
> +	ltr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> +	if (!ltr)
> +		return -EINVAL;
> +
> +	pci_read_config_word(pdev, ltr + state, &val);
> +
> +	return sysfs_emit(buf, "0x%0x\n", val);
> +}
> +
> +static ssize_t ltr_attr_store_common(struct device *dev,
> +			   struct device_attribute *attr,
> +			   const char *buf, size_t len, u8 state)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int ltr;
> +	u16 val;
> +
> +	ltr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> +	if (!ltr)
> +		return -EINVAL;
> +
> +	if (kstrtou16(buf, 16, &val) < 0)
> +		return -EINVAL;
> +
> +	/* LatencyScale is not permitted to be 110 or 111 */
> +	if ((val >> 10) > 5)
> +		return -EINVAL;
> +
> +	pci_write_config_word(pdev, ltr + state, val);
> +
> +	return len;
> +}
> +
> +#define LTR_ATTR(_f, _s)						\
> +static ssize_t _f##_show(struct device *dev,				\
> +			 struct device_attribute *attr, char *buf)	\
> +{ return ltr_attr_show_common(dev, attr, buf, PCI_LTR_##_s); }		\
> +									\
> +static ssize_t _f##_store(struct device *dev,				\
> +			  struct device_attribute *attr,		\
> +			  const char *buf, size_t len)			\
> +{ return ltr_attr_store_common(dev, attr, buf, len, PCI_LTR_##_s); }
> +
> +LTR_ATTR(ltr_max_snoop_lat, MAX_SNOOP_LAT);
> +LTR_ATTR(ltr_max_nosnoop_lat, MAX_NOSNOOP_LAT);
> +
>  static DEVICE_ATTR_RW(clkpm);
>  static DEVICE_ATTR_RW(l0s_aspm);
>  static DEVICE_ATTR_RW(l1_aspm);
> @@ -1306,6 +1359,8 @@ static DEVICE_ATTR_RW(l1_1_aspm);
>  static DEVICE_ATTR_RW(l1_2_aspm);
>  static DEVICE_ATTR_RW(l1_1_pcipm);
>  static DEVICE_ATTR_RW(l1_2_pcipm);
> +static DEVICE_ATTR_RW(ltr_max_snoop_lat);
> +static DEVICE_ATTR_RW(ltr_max_nosnoop_lat);
>  
>  static struct attribute *aspm_ctrl_attrs[] = {
>  	&dev_attr_clkpm.attr,
> @@ -1315,6 +1370,8 @@ static struct attribute *aspm_ctrl_attrs[] = {
>  	&dev_attr_l1_2_aspm.attr,
>  	&dev_attr_l1_1_pcipm.attr,
>  	&dev_attr_l1_2_pcipm.attr,
> +	&dev_attr_ltr_max_snoop_lat.attr,
> +	&dev_attr_ltr_max_nosnoop_lat.attr,
>  	NULL
>  };
>  
> @@ -1338,6 +1395,8 @@ static umode_t aspm_ctrl_attrs_are_visible(struct kobject *kobj,
>  
>  	if (n == 0)
>  		return link->clkpm_capable ? a->mode : 0;
> +	else if (n == 7 || n == 8)
> +		return pdev->ltr_path ? a->mode : 0;
>  
>  	return link->aspm_capable & aspm_state_map[n - 1] ? a->mode : 0;
>  }
> -- 
> 2.32.0
> 

Re: [PATCH v2 3/3] PCI/ASPM: Add LTR sysfs attributes

[Kai-Heng Feng]

On Thu, Oct 21, 2021 at 11:09 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Oct 21, 2021 at 11:51:59AM +0800, Kai-Heng Feng wrote:
> > Sometimes BIOS may not be able to program ASPM and LTR settings, for
> > instance, the NVMe devices behind Intel VMD bridges. For this case, both
> > ASPM and LTR have to be enabled to have significant power saving.
> >
> > Since we still want POLICY_DEFAULT honor the default BIOS ASPM settings,
> > introduce LTR sysfs knobs so users can set max snoop and max nosnoop
> > latency manually or via udev rules.
>
> How is the user supposed to figure out what "max snoop" and "max
> nosnoop" values to program?

Actually, the only way I know is to get the value from other OS.

>
> If we add this, I'm afraid we'll have people programming random things
> that seem to work but are not actually reliable.

IMO users need to take full responsibility for own doings.
Also, it's already doable by using setpci...

If we don't want to add LTR sysfs, what other options do we have to
enable VMD ASPM and LTR by default since BIOS doesn't do it for us?
1) Enable it in the PCI or VMD driver, however this approach violates
POLICY_DEFAULT.
2) Use `setpci` directly in udev rules to enable VMD ASPM and LTR.

I think 2) is bad, and since 1) isn't so good either, the approach in
this patch may be the best compromise.

Kai-Heng

>
> > [1] https://github.com/systemd/systemd/pull/17899/
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=209789
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > v2:
> >  - New patch.
> >
> >  drivers/pci/pcie/aspm.c | 59 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 1560859ab056..f7dc62936445 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1299,6 +1299,59 @@ static ssize_t clkpm_store(struct device *dev,
> >       return len;
> >  }
> >
> > +static ssize_t ltr_attr_show_common(struct device *dev,
> > +                       struct device_attribute *attr, char *buf, u8 state)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(dev);
> > +     int ltr;
> > +     u16 val;
> > +
> > +     ltr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > +     if (!ltr)
> > +             return -EINVAL;
> > +
> > +     pci_read_config_word(pdev, ltr + state, &val);
> > +
> > +     return sysfs_emit(buf, "0x%0x\n", val);
> > +}
> > +
> > +static ssize_t ltr_attr_store_common(struct device *dev,
> > +                        struct device_attribute *attr,
> > +                        const char *buf, size_t len, u8 state)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(dev);
> > +     int ltr;
> > +     u16 val;
> > +
> > +     ltr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > +     if (!ltr)
> > +             return -EINVAL;
> > +
> > +     if (kstrtou16(buf, 16, &val) < 0)
> > +             return -EINVAL;
> > +
> > +     /* LatencyScale is not permitted to be 110 or 111 */
> > +     if ((val >> 10) > 5)
> > +             return -EINVAL;
> > +
> > +     pci_write_config_word(pdev, ltr + state, val);
> > +
> > +     return len;
> > +}
> > +
> > +#define LTR_ATTR(_f, _s)                                             \
> > +static ssize_t _f##_show(struct device *dev,                         \
> > +                      struct device_attribute *attr, char *buf)      \
> > +{ return ltr_attr_show_common(dev, attr, buf, PCI_LTR_##_s); }               \
> > +                                                                     \
> > +static ssize_t _f##_store(struct device *dev,                                \
> > +                       struct device_attribute *attr,                \
> > +                       const char *buf, size_t len)                  \
> > +{ return ltr_attr_store_common(dev, attr, buf, len, PCI_LTR_##_s); }
> > +
> > +LTR_ATTR(ltr_max_snoop_lat, MAX_SNOOP_LAT);
> > +LTR_ATTR(ltr_max_nosnoop_lat, MAX_NOSNOOP_LAT);
> > +
> >  static DEVICE_ATTR_RW(clkpm);
> >  static DEVICE_ATTR_RW(l0s_aspm);
> >  static DEVICE_ATTR_RW(l1_aspm);
> > @@ -1306,6 +1359,8 @@ static DEVICE_ATTR_RW(l1_1_aspm);
> >  static DEVICE_ATTR_RW(l1_2_aspm);
> >  static DEVICE_ATTR_RW(l1_1_pcipm);
> >  static DEVICE_ATTR_RW(l1_2_pcipm);
> > +static DEVICE_ATTR_RW(ltr_max_snoop_lat);
> > +static DEVICE_ATTR_RW(ltr_max_nosnoop_lat);
> >
> >  static struct attribute *aspm_ctrl_attrs[] = {
> >       &dev_attr_clkpm.attr,
> > @@ -1315,6 +1370,8 @@ static struct attribute *aspm_ctrl_attrs[] = {
> >       &dev_attr_l1_2_aspm.attr,
> >       &dev_attr_l1_1_pcipm.attr,
> >       &dev_attr_l1_2_pcipm.attr,
> > +     &dev_attr_ltr_max_snoop_lat.attr,
> > +     &dev_attr_ltr_max_nosnoop_lat.attr,
> >       NULL
> >  };
> >
> > @@ -1338,6 +1395,8 @@ static umode_t aspm_ctrl_attrs_are_visible(struct kobject *kobj,
> >
> >       if (n == 0)
> >               return link->clkpm_capable ? a->mode : 0;
> > +     else if (n == 7 || n == 8)
> > +             return pdev->ltr_path ? a->mode : 0;
> >
> >       return link->aspm_capable & aspm_state_map[n - 1] ? a->mode : 0;
> >  }
> > --
> > 2.32.0
> >

Re: [PATCH v2 3/3] PCI/ASPM: Add LTR sysfs attributes

[Bjorn Helgaas]

On Mon, Oct 25, 2021 at 06:33:50PM +0800, Kai-Heng Feng wrote:
> On Thu, Oct 21, 2021 at 11:09 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Oct 21, 2021 at 11:51:59AM +0800, Kai-Heng Feng wrote:
> > > Sometimes BIOS may not be able to program ASPM and LTR settings, for
> > > instance, the NVMe devices behind Intel VMD bridges. For this case, both
> > > ASPM and LTR have to be enabled to have significant power saving.
> > >
> > > Since we still want POLICY_DEFAULT honor the default BIOS ASPM settings,
> > > introduce LTR sysfs knobs so users can set max snoop and max nosnoop
> > > latency manually or via udev rules.
> >
> > How is the user supposed to figure out what "max snoop" and "max
> > nosnoop" values to program?
> 
> Actually, the only way I know is to get the value from other OS.

I don't see how this can be a workable solution.  IIUC this is
specifically related to ASPM L1.2.  L1.2 depends on LTR (the max
snoop/nosnoop values) and the TPOWER_ON and Common_Mode_Restore_Time
values.  PCIe r5.0, sec 5.5.4, says:

  Prior to setting either or both of the enable bits for L1.2, the
  values for TPOWER_ON, Common_Mode_Restore_Time, and, if the ASPM
  L1.2 Enable bit is to be Set, the LTR_L1.2_THRESHOLD (both Value and
  Scale fields) must be programmed.

  The TPOWER_ON and Common_Mode_Restore_Time fields must be programmed
  to the appropriate values based on the components and AC coupling
  capacitors used in the connection linking the two components. The
  determination of these values is design implementation specific.

I do not think collecting values from some other OS is a reasonable
way to set these.  The values would apparently depend on the
electrical design of the particular machine.

> > If we add this, I'm afraid we'll have people programming random things
> > that seem to work but are not actually reliable.
> 
> IMO users need to take full responsibility for own doings.
> Also, it's already doable by using setpci...

I don't think it currently does, but setpci should taint the kernel.

If users want to write setpci scripts to fiddle with stuff, that's
great, but that moves it outside the supportable realm.  If we provide
a sysfs interface to do this, then it becomes more of *our* problem to
make it work correctly, and I don't think that's practical in this
case.

> If we don't want to add LTR sysfs, what other options do we have to
> enable VMD ASPM and LTR by default since BIOS doesn't do it for us?
> 1) Enable it in the PCI or VMD driver, however this approach violates
> POLICY_DEFAULT.
> 2) Use `setpci` directly in udev rules to enable VMD ASPM and LTR.
> 
> I think 2) is bad, and since 1) isn't so good either, the approach in
> this patch may be the best compromise.

I do not know how to safely enable L1.2.  It's likely that I'm just
missing something, but I don't see enough information in PCI config
space and the PCI Firmware interface (_DSM for Latency Tolerance
Reporting) to enable L1.2.  It's possible that a new firmware
interface is required.

I don't think it's wise to enable L1.2 unless we have good confidence
that we know how to do it correctly.  It's hard enough to debug ASPM
issues as it is.

Bjorn

Re: [PATCH v2 3/3] PCI/ASPM: Add LTR sysfs attributes

[Kai-Heng Feng]

On Tue, Oct 26, 2021 at 1:31 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, Oct 25, 2021 at 06:33:50PM +0800, Kai-Heng Feng wrote:
> > On Thu, Oct 21, 2021 at 11:09 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Thu, Oct 21, 2021 at 11:51:59AM +0800, Kai-Heng Feng wrote:
> > > > Sometimes BIOS may not be able to program ASPM and LTR settings, for
> > > > instance, the NVMe devices behind Intel VMD bridges. For this case, both
> > > > ASPM and LTR have to be enabled to have significant power saving.
> > > >
> > > > Since we still want POLICY_DEFAULT honor the default BIOS ASPM settings,
> > > > introduce LTR sysfs knobs so users can set max snoop and max nosnoop
> > > > latency manually or via udev rules.
> > >
> > > How is the user supposed to figure out what "max snoop" and "max
> > > nosnoop" values to program?
> >
> > Actually, the only way I know is to get the value from other OS.
>
> I don't see how this can be a workable solution.  IIUC this is
> specifically related to ASPM L1.2.  L1.2 depends on LTR (the max
> snoop/nosnoop values) and the TPOWER_ON and Common_Mode_Restore_Time
> values.  PCIe r5.0, sec 5.5.4, says:
>
>   Prior to setting either or both of the enable bits for L1.2, the
>   values for TPOWER_ON, Common_Mode_Restore_Time, and, if the ASPM
>   L1.2 Enable bit is to be Set, the LTR_L1.2_THRESHOLD (both Value and
>   Scale fields) must be programmed.
>
>   The TPOWER_ON and Common_Mode_Restore_Time fields must be programmed
>   to the appropriate values based on the components and AC coupling
>   capacitors used in the connection linking the two components. The
>   determination of these values is design implementation specific.
>
> I do not think collecting values from some other OS is a reasonable
> way to set these.  The values would apparently depend on the
> electrical design of the particular machine.

What if we fallback to the original approach and use the VMD driver to
enable ASPM and LTR values?
At least I think Intel should be able to provide correct values for their SoC.

>
> > > If we add this, I'm afraid we'll have people programming random things
> > > that seem to work but are not actually reliable.
> >
> > IMO users need to take full responsibility for own doings.
> > Also, it's already doable by using setpci...
>
> I don't think it currently does, but setpci should taint the kernel.
>
> If users want to write setpci scripts to fiddle with stuff, that's
> great, but that moves it outside the supportable realm.  If we provide
> a sysfs interface to do this, then it becomes more of *our* problem to
> make it work correctly, and I don't think that's practical in this
> case.

OK.

>
> > If we don't want to add LTR sysfs, what other options do we have to
> > enable VMD ASPM and LTR by default since BIOS doesn't do it for us?
> > 1) Enable it in the PCI or VMD driver, however this approach violates
> > POLICY_DEFAULT.
> > 2) Use `setpci` directly in udev rules to enable VMD ASPM and LTR.
> >
> > I think 2) is bad, and since 1) isn't so good either, the approach in
> > this patch may be the best compromise.
>
> I do not know how to safely enable L1.2.  It's likely that I'm just
> missing something, but I don't see enough information in PCI config
> space and the PCI Firmware interface (_DSM for Latency Tolerance
> Reporting) to enable L1.2.  It's possible that a new firmware
> interface is required.

I was told by Intel that they didn't use _DSM to get LTR values, at
least not the VMD case.

>
> I don't think it's wise to enable L1.2 unless we have good confidence
> that we know how to do it correctly.  It's hard enough to debug ASPM
> issues as it is.

So what other options do we have if we want to enable VMD ASPM while
keeping CONFIG_PCIEASPM_DEFAULT=y?
Right now we enabled the VMD ASPM/LTR bits in downstream kernel, but
other distro users may want to have upstream support for this.

Kai-Heng

>
> Bjorn

Re: [PATCH v2 3/3] PCI/ASPM: Add LTR sysfs attributes

[Bjorn Helgaas]

On Tue, Oct 26, 2021 at 10:28:38AM +0800, Kai-Heng Feng wrote:

> What if we fallback to the original approach and use the VMD driver
> to enable ASPM and LTR values?  At least I think Intel should be
> able to provide correct values for their SoC.

Can you post the patches for that?  I'm not sure exactly what the
original approach was.  Are these the same as the downstream support
you mention below?

> So what other options do we have if we want to enable VMD ASPM while
> keeping CONFIG_PCIEASPM_DEFAULT=y?  Right now we enabled the VMD
> ASPM/LTR bits in downstream kernel, but other distro users may want
> to have upstream support for this.

Re: [PATCH v2 3/3] PCI/ASPM: Add LTR sysfs attributes

[Kai-Heng Feng]

On Wed, Oct 27, 2021 at 12:53 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Oct 26, 2021 at 10:28:38AM +0800, Kai-Heng Feng wrote:
>
> > What if we fallback to the original approach and use the VMD driver
> > to enable ASPM and LTR values?  At least I think Intel should be
> > able to provide correct values for their SoC.
>
> Can you post the patches for that?  I'm not sure exactly what the
> original approach was.  Are these the same as the downstream support
> you mention below?

This is the previous attempt to enable VMD ASPM:
https://lore.kernel.org/linux-pci/20200930082455.25613-1-kai.heng.feng@canonical.com/

It didn't enable LTR though.

Right now the downstream kernel use PCI quirk to enable VMD ASPM and LTR:
https://kernel.ubuntu.com/git/ubuntu/unstable.git/commit/?id=069ab00c2613d27cb7cdeb2a4c751de89dab81b4
https://kernel.ubuntu.com/git/ubuntu/unstable.git/commit/?id=1e4dec5fe846f8dd8954b173434670f8ae30b5ff

The patches don't touch VMD driver to minimize merge conflict on VMD driver.
I really hope we can put these changes to upstream VMD driver.

Kai-Heng

>
> > So what other options do we have if we want to enable VMD ASPM while
> > keeping CONFIG_PCIEASPM_DEFAULT=y?  Right now we enabled the VMD
> > ASPM/LTR bits in downstream kernel, but other distro users may want
> > to have upstream support for this.