[PATCH 1/2] PCI/ASPM: Add helper to enable ASPM link

[PATCH 1/2] PCI/ASPM: Add helper to enable ASPM link

[Kai-Heng Feng]

Platform firmware may not be able to access config space to program
ASPM. For instance, devices behind Intel VMD are not configured by the
BIOS.

So add a helper to let drivers have an option to enable ASPM.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/pcie/aspm.c | 73 ++++++++++++++++++++++++++++++-----------
 include/linux/pci.h     |  7 ++++
 2 files changed, 60 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 253c30cc1967..b5ac6a99e016 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1080,6 +1080,58 @@ static struct pcie_link_state *pcie_aspm_get_link(struct pci_dev *pdev)
 	return bridge->link_state;
 }
 
+static u32 pcie_aspm_convert_state(int state)
+{
+	u32 aspm = 0;
+
+	if (state & PCIE_LINK_STATE_L0S)
+		aspm |= ASPM_STATE_L0S;
+	if (state & PCIE_LINK_STATE_L1)
+		/* L1 PM substates require L1 */
+		aspm |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
+	if (state & PCIE_LINK_STATE_L1_1)
+		aspm |= ASPM_STATE_L1_1;
+	if (state & PCIE_LINK_STATE_L1_2)
+		aspm |= ASPM_STATE_L1_2;
+	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
+		aspm |= ASPM_STATE_L1_1_PCIPM;
+	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
+		aspm |= ASPM_STATE_L1_2_PCIPM;
+
+	return aspm;
+}
+
+static void pci_set_link_state(struct pci_dev *pdev, bool enable, int state)
+{
+	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
+
+	mutex_lock(&aspm_lock);
+	if (enable)
+		link->aspm_default = pcie_aspm_convert_state(state);
+	else
+		link->aspm_disable = pcie_aspm_convert_state(state);
+	pcie_config_aspm_link(link, policy_to_aspm_state(link));
+
+	if (state & PCIE_LINK_STATE_CLKPM) {
+		link->clkpm_default = enable;
+		link->clkpm_disable = !enable;
+	}
+	pcie_set_clkpm(link, policy_to_clkpm_state(link));
+	mutex_unlock(&aspm_lock);
+}
+
+int pci_enable_link_state(struct pci_dev *pdev, int state)
+{
+	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
+
+	if (!link)
+		return -EINVAL;
+
+	pci_set_link_state(pdev, true, state);
+	return 0;
+}
+EXPORT_SYMBOL(pci_enable_link_state);
+
 static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
 {
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
@@ -1101,26 +1153,7 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
 
 	if (sem)
 		down_read(&pci_bus_sem);
-	mutex_lock(&aspm_lock);
-	if (state & PCIE_LINK_STATE_L0S)
-		link->aspm_disable |= ASPM_STATE_L0S;
-	if (state & PCIE_LINK_STATE_L1)
-		/* L1 PM substates require L1 */
-		link->aspm_disable |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
-	if (state & PCIE_LINK_STATE_L1_1)
-		link->aspm_disable |= ASPM_STATE_L1_1;
-	if (state & PCIE_LINK_STATE_L1_2)
-		link->aspm_disable |= ASPM_STATE_L1_2;
-	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
-		link->aspm_disable |= ASPM_STATE_L1_1_PCIPM;
-	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
-		link->aspm_disable |= ASPM_STATE_L1_2_PCIPM;
-	pcie_config_aspm_link(link, policy_to_aspm_state(link));
-
-	if (state & PCIE_LINK_STATE_CLKPM)
-		link->clkpm_disable = 1;
-	pcie_set_clkpm(link, policy_to_clkpm_state(link));
-	mutex_unlock(&aspm_lock);
+	pci_set_link_state(pdev, false, state);
 	if (sem)
 		up_read(&pci_bus_sem);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 835530605c0d..a6a7830ec258 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1568,14 +1568,21 @@ extern bool pcie_ports_native;
 #define PCIE_LINK_STATE_L1_2		BIT(4)
 #define PCIE_LINK_STATE_L1_1_PCIPM	BIT(5)
 #define PCIE_LINK_STATE_L1_2_PCIPM	BIT(6)
+#define PCIE_LINK_STATE_ALL		(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
+					 PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
+					 PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
+					 PCIE_LINK_STATE_L1_2_PCIPM)
 
 #ifdef CONFIG_PCIEASPM
+int pci_enable_link_state(struct pci_dev *pdev, int state);
 int pci_disable_link_state(struct pci_dev *pdev, int state);
 int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
 void pcie_no_aspm(void);
 bool pcie_aspm_support_enabled(void);
 bool pcie_aspm_enabled(struct pci_dev *pdev);
 #else
+static inline int pci_enable_link_state(struct pci_dev *pdev, int state)
+{ return 0; }
 static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
 { return 0; }
 static inline int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
-- 
2.17.1

[PATCH 2/2] PCI: vmd: Enable ASPM for mobile platforms

[Kai-Heng Feng]

BIOS may not be able to program ASPM for links behind VMD, prevent Intel
SoC from entering deeper power saving state.

So enable ASPM for links behind VMD to increase battery life.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/controller/vmd.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index f69ef8c89f72..058fdef9c566 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -417,6 +417,22 @@ static int vmd_find_free_domain(void)
 	return domain + 1;
 }
 
+static const struct pci_device_id vmd_mobile_bridge_tbl[] = {
+	{ PCI_VDEVICE(INTEL, 0x9a09) },
+	{ PCI_VDEVICE(INTEL, 0xa0b0) },
+	{ PCI_VDEVICE(INTEL, 0xa0bc) },
+	{ }
+};
+
+static int vmd_enable_aspm(struct device *dev, void *data)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
+
+	return 0;
+}
+
 static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
@@ -603,8 +619,12 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	 * and will fail pcie_bus_configure_settings() early. It can instead be
 	 * run on each of the real root ports.
 	 */
-	list_for_each_entry(child, &vmd->bus->children, node)
+	list_for_each_entry(child, &vmd->bus->children, node) {
+		if (pci_match_id(vmd_mobile_bridge_tbl, child->self))
+			device_for_each_child(&child->self->dev, NULL, vmd_enable_aspm);
+
 		pcie_bus_configure_settings(child);
+	}
 
 	pci_bus_add_devices(vmd->bus);
 
-- 
2.17.1

Re: [PATCH 2/2] PCI: vmd: Enable ASPM for mobile platforms

[Bjorn Helgaas]

On Wed, Sep 30, 2020 at 04:24:54PM +0800, Kai-Heng Feng wrote:
> BIOS may not be able to program ASPM for links behind VMD, prevent Intel
> SoC from entering deeper power saving state.

It's not a question of BIOS not being *able* to configure ASPM.  I
think BIOS could do it, at least in principle, if it had a driver for
VMD.  Actually, it probably *does* include some sort of VMD code
because it sounds like BIOS can assign some Root Ports to appear
either as regular Root Ports or behind the VMD.

Since this issue is directly related to the unusual VMD topology, I
think it would be worth a quick recap here.  Maybe something like:

  VMD is a Root Complex Integrated Endpoint that acts as a host bridge
  to a secondary PCIe domain.  BIOS can reassign one or more Root
  Ports to appear within a VMD domain instead of the primary domain.

  However, BIOS may not enable ASPM for the hierarchies behind a VMD,
  ...

(This is based on the commit log from 185a383ada2e ("x86/PCI: Add
driver for Intel Volume Management Device (VMD)")).

But we still have the problem that CONFIG_PCIEASPM_DEFAULT=y means
"use the BIOS defaults", and this patch would make it so we use the
BIOS defaults *except* for things behind VMD.

  - Why should VMD be a special case?

  - How would we document such a special case?

  - If we built with CONFIG_PCIEASPM_POWERSAVE=y, would that solve the
    SoC power state problem?

  - What issues would CONFIG_PCIEASPM_POWERSAVE=y introduce?

Link to previous discussion for the archives:
https://lore.kernel.org/r/49A36179-D336-4A5E-8B7A-A632833AE6B2@canonical.com

> So enable ASPM for links behind VMD to increase battery life.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/pci/controller/vmd.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index f69ef8c89f72..058fdef9c566 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -417,6 +417,22 @@ static int vmd_find_free_domain(void)
>  	return domain + 1;
>  }
>  
> +static const struct pci_device_id vmd_mobile_bridge_tbl[] = {
> +	{ PCI_VDEVICE(INTEL, 0x9a09) },
> +	{ PCI_VDEVICE(INTEL, 0xa0b0) },
> +	{ PCI_VDEVICE(INTEL, 0xa0bc) },
> +	{ }
> +};
> +
> +static int vmd_enable_aspm(struct device *dev, void *data)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
> +
> +	return 0;
> +}
> +
>  static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  {
>  	struct pci_sysdata *sd = &vmd->sysdata;
> @@ -603,8 +619,12 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  	 * and will fail pcie_bus_configure_settings() early. It can instead be
>  	 * run on each of the real root ports.
>  	 */
> -	list_for_each_entry(child, &vmd->bus->children, node)
> +	list_for_each_entry(child, &vmd->bus->children, node) {
> +		if (pci_match_id(vmd_mobile_bridge_tbl, child->self))
> +			device_for_each_child(&child->self->dev, NULL, vmd_enable_aspm);

Wouldn't something like this be sufficient?

  list_for_each_entry(dev, &child->devices, bus_list)
    vmd_enable_aspm(dev);

>  		pcie_bus_configure_settings(child);
> +	}
>  
>  	pci_bus_add_devices(vmd->bus);
>  
> -- 
> 2.17.1
> 

Re: [PATCH 2/2] PCI: vmd: Enable ASPM for mobile platforms

[Kai-Heng Feng]

Hi Bjorn,

> On Oct 3, 2020, at 06:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Wed, Sep 30, 2020 at 04:24:54PM +0800, Kai-Heng Feng wrote:
>> BIOS may not be able to program ASPM for links behind VMD, prevent Intel
>> SoC from entering deeper power saving state.
> 
> It's not a question of BIOS not being *able* to configure ASPM.  I
> think BIOS could do it, at least in principle, if it had a driver for
> VMD.  Actually, it probably *does* include some sort of VMD code
> because it sounds like BIOS can assign some Root Ports to appear
> either as regular Root Ports or behind the VMD.
> 
> Since this issue is directly related to the unusual VMD topology, I
> think it would be worth a quick recap here.  Maybe something like:
> 
>  VMD is a Root Complex Integrated Endpoint that acts as a host bridge
>  to a secondary PCIe domain.  BIOS can reassign one or more Root
>  Ports to appear within a VMD domain instead of the primary domain.
> 
>  However, BIOS may not enable ASPM for the hierarchies behind a VMD,
>  ...
> 
> (This is based on the commit log from 185a383ada2e ("x86/PCI: Add
> driver for Intel Volume Management Device (VMD)")).

Ok, will just copy the portion as-is if there's patch v2 :)

> 
> But we still have the problem that CONFIG_PCIEASPM_DEFAULT=y means
> "use the BIOS defaults", and this patch would make it so we use the
> BIOS defaults *except* for things behind VMD.
> 
>  - Why should VMD be a special case?

Because BIOS doesn't handle ASPM for it so it's up to software to do the job.
In the meantime we want other devices still use the BIOS defaults to not introduce any regression.

> 
>  - How would we document such a special case?

I wonder whether other devices that add PCIe domain have the same behavior?
Maybe it's not a special case at all...

I understand the end goal is to keep consistency for the entire ASPM logic. However I can't think of any possible solution right now.

> 
>  - If we built with CONFIG_PCIEASPM_POWERSAVE=y, would that solve the
>    SoC power state problem?

Yes.

> 
>  - What issues would CONFIG_PCIEASPM_POWERSAVE=y introduce?

This will break many systems, at least for the 1st Gen Ryzen desktops and laptops.
All PCIe ASPM are not enabled by BIOS, and those systems immediately freeze once ASPM is enabled.

Kai-Heng

> 
> Link to previous discussion for the archives:
> https://lore.kernel.org/r/49A36179-D336-4A5E-8B7A-A632833AE6B2@canonical.com
> 
>> So enable ASPM for links behind VMD to increase battery life.
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/pci/controller/vmd.c | 22 +++++++++++++++++++++-
>> 1 file changed, 21 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index f69ef8c89f72..058fdef9c566 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -417,6 +417,22 @@ static int vmd_find_free_domain(void)
>> 	return domain + 1;
>> }
>> 
>> +static const struct pci_device_id vmd_mobile_bridge_tbl[] = {
>> +	{ PCI_VDEVICE(INTEL, 0x9a09) },
>> +	{ PCI_VDEVICE(INTEL, 0xa0b0) },
>> +	{ PCI_VDEVICE(INTEL, 0xa0bc) },
>> +	{ }
>> +};
>> +
>> +static int vmd_enable_aspm(struct device *dev, void *data)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
>> +
>> +	return 0;
>> +}
>> +
>> static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>> {
>> 	struct pci_sysdata *sd = &vmd->sysdata;
>> @@ -603,8 +619,12 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>> 	 * and will fail pcie_bus_configure_settings() early. It can instead be
>> 	 * run on each of the real root ports.
>> 	 */
>> -	list_for_each_entry(child, &vmd->bus->children, node)
>> +	list_for_each_entry(child, &vmd->bus->children, node) {
>> +		if (pci_match_id(vmd_mobile_bridge_tbl, child->self))
>> +			device_for_each_child(&child->self->dev, NULL, vmd_enable_aspm);
> 
> Wouldn't something like this be sufficient?
> 
>  list_for_each_entry(dev, &child->devices, bus_list)
>    vmd_enable_aspm(dev);
> 
>> 		pcie_bus_configure_settings(child);
>> +	}
>> 
>> 	pci_bus_add_devices(vmd->bus);
>> 
>> -- 
>> 2.17.1

Re: [PATCH 2/2] PCI: vmd: Enable ASPM for mobile platforms

[Bjorn Helgaas]

[+cc Ian, who's also working on an ASPM issue]

On Tue, Oct 06, 2020 at 02:40:32AM +0800, Kai-Heng Feng wrote:
> > On Oct 3, 2020, at 06:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Sep 30, 2020 at 04:24:54PM +0800, Kai-Heng Feng wrote:
> >> BIOS may not be able to program ASPM for links behind VMD, prevent Intel
> >> SoC from entering deeper power saving state.
> > 
> > It's not a question of BIOS not being *able* to configure ASPM.  I
> > think BIOS could do it, at least in principle, if it had a driver for
> > VMD.  Actually, it probably *does* include some sort of VMD code
> > because it sounds like BIOS can assign some Root Ports to appear
> > either as regular Root Ports or behind the VMD.
> > 
> > Since this issue is directly related to the unusual VMD topology, I
> > think it would be worth a quick recap here.  Maybe something like:
> > 
> >  VMD is a Root Complex Integrated Endpoint that acts as a host bridge
> >  to a secondary PCIe domain.  BIOS can reassign one or more Root
> >  Ports to appear within a VMD domain instead of the primary domain.
> > 
> >  However, BIOS may not enable ASPM for the hierarchies behind a VMD,
> >  ...
> > 
> > (This is based on the commit log from 185a383ada2e ("x86/PCI: Add
> > driver for Intel Volume Management Device (VMD)")).
> 
> Ok, will just copy the portion as-is if there's patch v2 :)
> 
> > But we still have the problem that CONFIG_PCIEASPM_DEFAULT=y means
> > "use the BIOS defaults", and this patch would make it so we use the
> > BIOS defaults *except* for things behind VMD.
> > 
> >  - Why should VMD be a special case?
> 
> Because BIOS doesn't handle ASPM for it so it's up to software to do
> the job.  In the meantime we want other devices still use the BIOS
> defaults to not introduce any regression.
> 
> >  - How would we document such a special case?
> 
> I wonder whether other devices that add PCIe domain have the same
> behavior?  Maybe it's not a special case at all...

What other devices are these?

> I understand the end goal is to keep consistency for the entire ASPM
> logic. However I can't think of any possible solution right now.
> 
> >  - If we built with CONFIG_PCIEASPM_POWERSAVE=y, would that solve the
> >    SoC power state problem?
> 
> Yes.
> 
> >  - What issues would CONFIG_PCIEASPM_POWERSAVE=y introduce?
> 
> This will break many systems, at least for the 1st Gen Ryzen
> desktops and laptops.
>
> All PCIe ASPM are not enabled by BIOS, and those systems immediately
> freeze once ASPM is enabled.

That indicates a defect in the Linux ASPM code.  We should fix that.
It should be safe to use CONFIG_PCIEASPM_POWERSAVE=y on every system.

Are there bug reports for these?  The info we would need to start with
includes "lspci -vv" and dmesg log (with CONFIG_PCIEASPM_DEFAULT=y).
If a console log with CONFIG_PCIEASPM_POWERSAVE=y is available, that
might be interesting, too.  We'll likely need to add some
instrumentation and do some experimentation, but in principle, this
should be fixable.

Bjorn

Re: [PATCH 2/2] PCI: vmd: Enable ASPM for mobile platforms

[Kai-Heng Feng]

> On Oct 6, 2020, at 03:19, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> [+cc Ian, who's also working on an ASPM issue]
> 
> On Tue, Oct 06, 2020 at 02:40:32AM +0800, Kai-Heng Feng wrote:
>>> On Oct 3, 2020, at 06:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Wed, Sep 30, 2020 at 04:24:54PM +0800, Kai-Heng Feng wrote:
>>>> BIOS may not be able to program ASPM for links behind VMD, prevent Intel
>>>> SoC from entering deeper power saving state.
>>> 
>>> It's not a question of BIOS not being *able* to configure ASPM.  I
>>> think BIOS could do it, at least in principle, if it had a driver for
>>> VMD.  Actually, it probably *does* include some sort of VMD code
>>> because it sounds like BIOS can assign some Root Ports to appear
>>> either as regular Root Ports or behind the VMD.
>>> 
>>> Since this issue is directly related to the unusual VMD topology, I
>>> think it would be worth a quick recap here.  Maybe something like:
>>> 
>>> VMD is a Root Complex Integrated Endpoint that acts as a host bridge
>>> to a secondary PCIe domain.  BIOS can reassign one or more Root
>>> Ports to appear within a VMD domain instead of the primary domain.
>>> 
>>> However, BIOS may not enable ASPM for the hierarchies behind a VMD,
>>> ...
>>> 
>>> (This is based on the commit log from 185a383ada2e ("x86/PCI: Add
>>> driver for Intel Volume Management Device (VMD)")).
>> 
>> Ok, will just copy the portion as-is if there's patch v2 :)
>> 
>>> But we still have the problem that CONFIG_PCIEASPM_DEFAULT=y means
>>> "use the BIOS defaults", and this patch would make it so we use the
>>> BIOS defaults *except* for things behind VMD.
>>> 
>>> - Why should VMD be a special case?
>> 
>> Because BIOS doesn't handle ASPM for it so it's up to software to do
>> the job.  In the meantime we want other devices still use the BIOS
>> defaults to not introduce any regression.
>> 
>>> - How would we document such a special case?
>> 
>> I wonder whether other devices that add PCIe domain have the same
>> behavior?  Maybe it's not a special case at all...
> 
> What other devices are these?

Controllers which add PCIe domain.

> 
>> I understand the end goal is to keep consistency for the entire ASPM
>> logic. However I can't think of any possible solution right now.
>> 
>>> - If we built with CONFIG_PCIEASPM_POWERSAVE=y, would that solve the
>>>   SoC power state problem?
>> 
>> Yes.
>> 
>>> - What issues would CONFIG_PCIEASPM_POWERSAVE=y introduce?
>> 
>> This will break many systems, at least for the 1st Gen Ryzen
>> desktops and laptops.
>> 
>> All PCIe ASPM are not enabled by BIOS, and those systems immediately
>> freeze once ASPM is enabled.
> 
> That indicates a defect in the Linux ASPM code.  We should fix that.
> It should be safe to use CONFIG_PCIEASPM_POWERSAVE=y on every system.

On those systems ASPM are also not enabled on Windows. So I think ASPM are disabled for a reason.

> 
> Are there bug reports for these? The info we would need to start with
> includes "lspci -vv" and dmesg log (with CONFIG_PCIEASPM_DEFAULT=y).
> If a console log with CONFIG_PCIEASPM_POWERSAVE=y is available, that
> might be interesting, too.  We'll likely need to add some
> instrumentation and do some experimentation, but in principle, this
> should be fixable.

Doing this is asking users to use hardware settings that ODM/OEM never tested, and I think the risk is really high.

Kai-Heng

> 
> Bjorn

Re: [PATCH 2/2] PCI: vmd: Enable ASPM for mobile platforms

[Ian Kumlien]

On Wed, Oct 7, 2020 at 6:26 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
>
>
> > On Oct 6, 2020, at 03:19, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+cc Ian, who's also working on an ASPM issue]
> >
> > On Tue, Oct 06, 2020 at 02:40:32AM +0800, Kai-Heng Feng wrote:
> >>> On Oct 3, 2020, at 06:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>> On Wed, Sep 30, 2020 at 04:24:54PM +0800, Kai-Heng Feng wrote:
> >>>> BIOS may not be able to program ASPM for links behind VMD, prevent Intel
> >>>> SoC from entering deeper power saving state.
> >>>
> >>> It's not a question of BIOS not being *able* to configure ASPM.  I
> >>> think BIOS could do it, at least in principle, if it had a driver for
> >>> VMD.  Actually, it probably *does* include some sort of VMD code
> >>> because it sounds like BIOS can assign some Root Ports to appear
> >>> either as regular Root Ports or behind the VMD.
> >>>
> >>> Since this issue is directly related to the unusual VMD topology, I
> >>> think it would be worth a quick recap here.  Maybe something like:
> >>>
> >>> VMD is a Root Complex Integrated Endpoint that acts as a host bridge
> >>> to a secondary PCIe domain.  BIOS can reassign one or more Root
> >>> Ports to appear within a VMD domain instead of the primary domain.
> >>>
> >>> However, BIOS may not enable ASPM for the hierarchies behind a VMD,
> >>> ...
> >>>
> >>> (This is based on the commit log from 185a383ada2e ("x86/PCI: Add
> >>> driver for Intel Volume Management Device (VMD)")).
> >>
> >> Ok, will just copy the portion as-is if there's patch v2 :)
> >>
> >>> But we still have the problem that CONFIG_PCIEASPM_DEFAULT=y means
> >>> "use the BIOS defaults", and this patch would make it so we use the
> >>> BIOS defaults *except* for things behind VMD.
> >>>
> >>> - Why should VMD be a special case?
> >>
> >> Because BIOS doesn't handle ASPM for it so it's up to software to do
> >> the job.  In the meantime we want other devices still use the BIOS
> >> defaults to not introduce any regression.
> >>
> >>> - How would we document such a special case?
> >>
> >> I wonder whether other devices that add PCIe domain have the same
> >> behavior?  Maybe it's not a special case at all...
> >
> > What other devices are these?
>
> Controllers which add PCIe domain.
>
> >
> >> I understand the end goal is to keep consistency for the entire ASPM
> >> logic. However I can't think of any possible solution right now.
> >>
> >>> - If we built with CONFIG_PCIEASPM_POWERSAVE=y, would that solve the
> >>>   SoC power state problem?
> >>
> >> Yes.
> >>
> >>> - What issues would CONFIG_PCIEASPM_POWERSAVE=y introduce?
> >>
> >> This will break many systems, at least for the 1st Gen Ryzen
> >> desktops and laptops.
> >>
> >> All PCIe ASPM are not enabled by BIOS, and those systems immediately
> >> freeze once ASPM is enabled.
> >
> > That indicates a defect in the Linux ASPM code.  We should fix that.
> > It should be safe to use CONFIG_PCIEASPM_POWERSAVE=y on every system.
>
> On those systems ASPM are also not enabled on Windows. So I think ASPM are disabled for a reason.
>
> >
> > Are there bug reports for these? The info we would need to start with
> > includes "lspci -vv" and dmesg log (with CONFIG_PCIEASPM_DEFAULT=y).
> > If a console log with CONFIG_PCIEASPM_POWERSAVE=y is available, that
> > might be interesting, too.  We'll likely need to add some
> > instrumentation and do some experimentation, but in principle, this
> > should be fixable.
>
> Doing this is asking users to use hardware settings that ODM/OEM never tested, and I think the risk is really high.

They have to test it to comply with pcie specs? and what we're
currently doing is wrong...

This fixes the L1 behaviour in the kernel, could you test it?

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 253c30cc1967..893b37669087 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -434,7 +434,7 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,

 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 {
-       u32 latency, l1_switch_latency = 0;
+       u32 latency, l1_max_latency = 0, l1_switch_latency = 0;
        struct aspm_latency *acceptable;
        struct pcie_link_state *link;

@@ -456,10 +456,14 @@ static void pcie_aspm_check_latency(struct
pci_dev *endpoint)
                if ((link->aspm_capable & ASPM_STATE_L0S_DW) &&
                    (link->latency_dw.l0s > acceptable->l0s))
                        link->aspm_capable &= ~ASPM_STATE_L0S_DW;
+
                /*
                 * Check L1 latency.
-                * Every switch on the path to root complex need 1
-                * more microsecond for L1. Spec doesn't mention L0s.
+                *
+                * PCIe r5.0, sec 5.4.1.2.2 states:
+                * A Switch is required to initiate an L1 exit transition on its
+                * Upstream Port Link after no more than 1 μs from the
beginning of an
+                * L1 exit transition on any of its Downstream Port Links.
                 *
                 * The exit latencies for L1 substates are not advertised
                 * by a device.  Since the spec also doesn't mention a way
@@ -469,11 +473,14 @@ static void pcie_aspm_check_latency(struct
pci_dev *endpoint)
                 * L1 exit latencies advertised by a device include L1
                 * substate latencies (and hence do not do any check).
                 */
-               latency = max_t(u32, link->latency_up.l1, link->latency_dw.l1);
-               if ((link->aspm_capable & ASPM_STATE_L1) &&
-                   (latency + l1_switch_latency > acceptable->l1))
-                       link->aspm_capable &= ~ASPM_STATE_L1;
-               l1_switch_latency += 1000;
+               if (link->aspm_capable & ASPM_STATE_L1) {
+                       latency = max_t(u32, link->latency_up.l1,
link->latency_dw.l1);
+                       l1_max_latency = max_t(u32, latency, l1_max_latency);
+                       if (l1_max_latency + l1_switch_latency > acceptable->l1)
+                               link->aspm_capable &= ~ASPM_STATE_L1;
+
+                       l1_switch_latency += 1000;
+               }

                link = link->parent;
        }
---

If it doesn't you could also look at the following L0s patch

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 893b37669087..15d64832a988 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -434,7 +434,8 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,

 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 {
-       u32 latency, l1_max_latency = 0, l1_switch_latency = 0;
+       u32 latency, l1_max_latency = 0, l1_switch_latency = 0,
+               l0s_latency_up = 0, l0s_latency_dw = 0;
        struct aspm_latency *acceptable;
        struct pcie_link_state *link;

@@ -448,14 +449,18 @@ static void pcie_aspm_check_latency(struct
pci_dev *endpoint)

        while (link) {
                /* Check upstream direction L0s latency */
-               if ((link->aspm_capable & ASPM_STATE_L0S_UP) &&
-                   (link->latency_up.l0s > acceptable->l0s))
-                       link->aspm_capable &= ~ASPM_STATE_L0S_UP;
+               if (link->aspm_capable & ASPM_STATE_L0S_UP) {
+                       l0s_latency_up += link->latency_up.l0s;
+                       if (l0s_latency_up > acceptable->l0s)
+                               link->aspm_capable &= ~ASPM_STATE_L0S_UP;
+               }

                /* Check downstream direction L0s latency */
-               if ((link->aspm_capable & ASPM_STATE_L0S_DW) &&
-                   (link->latency_dw.l0s > acceptable->l0s))
-                       link->aspm_capable &= ~ASPM_STATE_L0S_DW;
+               if (link->aspm_capable & ASPM_STATE_L0S_DW) {
+                       l0s_latency_dw += link->latency_dw.l0s;
+                       if (l0s_latency_dw > acceptable->l0s)
+                               link->aspm_capable &= ~ASPM_STATE_L0S_DW;
+               }

                /*
                 * Check L1 latency.
---

I can send them directly as well if you prefer (i hope the client
doesn't mangle them)

> Kai-Heng
>
> >
> > Bjorn
>

Re: [PATCH 2/2] PCI: vmd: Enable ASPM for mobile platforms

[Bjorn Helgaas]

On Wed, Oct 07, 2020 at 12:26:19PM +0800, Kai-Heng Feng wrote:
> > On Oct 6, 2020, at 03:19, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Oct 06, 2020 at 02:40:32AM +0800, Kai-Heng Feng wrote:
> >>> On Oct 3, 2020, at 06:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>> On Wed, Sep 30, 2020 at 04:24:54PM +0800, Kai-Heng Feng wrote:

...
> >> I wonder whether other devices that add PCIe domain have the same
> >> behavior?  Maybe it's not a special case at all...
> > 
> > What other devices are these?
> 
> Controllers which add PCIe domain.

I was looking for specific examples, not just a restatement of what
you said before.  I'm just curious because there are a lot of
controllers I'm not familiar with, and I can't think of an example.

> >> I understand the end goal is to keep consistency for the entire ASPM
> >> logic. However I can't think of any possible solution right now.
> >> 
> >>> - If we built with CONFIG_PCIEASPM_POWERSAVE=y, would that solve the
> >>>   SoC power state problem?
> >> 
> >> Yes.
> >> 
> >>> - What issues would CONFIG_PCIEASPM_POWERSAVE=y introduce?
> >> 
> >> This will break many systems, at least for the 1st Gen Ryzen
> >> desktops and laptops.
> >> 
> >> All PCIe ASPM are not enabled by BIOS, and those systems immediately
> >> freeze once ASPM is enabled.
> > 
> > That indicates a defect in the Linux ASPM code.  We should fix that.
> > It should be safe to use CONFIG_PCIEASPM_POWERSAVE=y on every system.
> 
> On those systems ASPM are also not enabled on Windows. So I think
> ASPM are disabled for a reason.

If the platform knows ASPM needs to be disabled, it should be using
ACPI_FADT_NO_ASPM or _OSC to prevent the OS from using it.  And if
CONFIG_PCIEASPM_POWERSAVE=y means Linux enables ASPM when it
shouldn't, that's a Linux bug that we need to fix.

> > Are there bug reports for these? The info we would need to start with
> > includes "lspci -vv" and dmesg log (with CONFIG_PCIEASPM_DEFAULT=y).
> > If a console log with CONFIG_PCIEASPM_POWERSAVE=y is available, that
> > might be interesting, too.  We'll likely need to add some
> > instrumentation and do some experimentation, but in principle, this
> > should be fixable.
> 
> Doing this is asking users to use hardware settings that ODM/OEM
> never tested, and I think the risk is really high.

What?  That's not what I said at all.  I'm asking for information
about these hangs so we can fix them.  I'm not suggesting that you
should switch to CONFIG_PCIEASPM_POWERSAVE=y for the distro.

Let's back up.  You said:

  CONFIG_PCIEASPM_POWERSAVE=y ... will break many systems, at least
  for the 1st Gen Ryzen desktops and laptops.

  All PCIe ASPM are not enabled by BIOS, and those systems immediately
  freeze once ASPM is enabled.

These system hangs might be caused by (1) some hardware issue that
causes a hang when ASPM is enabled even if it is configured correctly
or (2) Linux configuring ASPM incorrectly.

For case (1), the platform should be using ACPI_FADT_NO_ASPM or _OSC
to prevent the OS from enabling ASPM.  Linux should pay attention to
that even when CONFIG_PCIEASPM_POWERSAVE=y.

If the platform *should* use these mechanisms but doesn't, the
solution is a quirk, not the folklore that "we can't use
CONFIG_PCIEASPM_POWERSAVE=y because it breaks some systems."

For case (2), we should fix Linux so it configures ASPM correctly.

We cannot use the build-time CONFIG_PCIEASPM settings to avoid these
hangs.  We need to fix the Linux run-time code so the system operates
correctly no matter what CONFIG_PCIEASPM setting is used.

We have sysfs knobs to control ASPM (see 72ea91afbfb0 ("PCI/ASPM: Add
sysfs attributes for controlling ASPM link states")).  They can do the
same thing at run-time as CONFIG_PCIEASPM_POWERSAVE=y does at
build-time.  If those knobs cause hangs on 1st Gen Ryzen systems, we
need to fix that.

Bjorn

Re: [PATCH 2/2] PCI: vmd: Enable ASPM for mobile platforms

[Ian Kumlien]

On Wed, Oct 7, 2020 at 3:30 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Oct 07, 2020 at 12:26:19PM +0800, Kai-Heng Feng wrote:
> > > On Oct 6, 2020, at 03:19, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Tue, Oct 06, 2020 at 02:40:32AM +0800, Kai-Heng Feng wrote:
> > >>> On Oct 3, 2020, at 06:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >>> On Wed, Sep 30, 2020 at 04:24:54PM +0800, Kai-Heng Feng wrote:
>
> ...
> > >> I wonder whether other devices that add PCIe domain have the same
> > >> behavior?  Maybe it's not a special case at all...
> > >
> > > What other devices are these?
> >
> > Controllers which add PCIe domain.
>
> I was looking for specific examples, not just a restatement of what
> you said before.  I'm just curious because there are a lot of
> controllers I'm not familiar with, and I can't think of an example.
>
> > >> I understand the end goal is to keep consistency for the entire ASPM
> > >> logic. However I can't think of any possible solution right now.
> > >>
> > >>> - If we built with CONFIG_PCIEASPM_POWERSAVE=y, would that solve the
> > >>>   SoC power state problem?
> > >>
> > >> Yes.
> > >>
> > >>> - What issues would CONFIG_PCIEASPM_POWERSAVE=y introduce?
> > >>
> > >> This will break many systems, at least for the 1st Gen Ryzen
> > >> desktops and laptops.
> > >>
> > >> All PCIe ASPM are not enabled by BIOS, and those systems immediately
> > >> freeze once ASPM is enabled.
> > >
> > > That indicates a defect in the Linux ASPM code.  We should fix that.
> > > It should be safe to use CONFIG_PCIEASPM_POWERSAVE=y on every system.
> >
> > On those systems ASPM are also not enabled on Windows. So I think
> > ASPM are disabled for a reason.
>
> If the platform knows ASPM needs to be disabled, it should be using
> ACPI_FADT_NO_ASPM or _OSC to prevent the OS from using it.  And if
> CONFIG_PCIEASPM_POWERSAVE=y means Linux enables ASPM when it
> shouldn't, that's a Linux bug that we need to fix.
>
> > > Are there bug reports for these? The info we would need to start with
> > > includes "lspci -vv" and dmesg log (with CONFIG_PCIEASPM_DEFAULT=y).
> > > If a console log with CONFIG_PCIEASPM_POWERSAVE=y is available, that
> > > might be interesting, too.  We'll likely need to add some
> > > instrumentation and do some experimentation, but in principle, this
> > > should be fixable.
> >
> > Doing this is asking users to use hardware settings that ODM/OEM
> > never tested, and I think the risk is really high.
>
> What?  That's not what I said at all.  I'm asking for information
> about these hangs so we can fix them.  I'm not suggesting that you
> should switch to CONFIG_PCIEASPM_POWERSAVE=y for the distro.
>
> Let's back up.  You said:
>
>   CONFIG_PCIEASPM_POWERSAVE=y ... will break many systems, at least
>   for the 1st Gen Ryzen desktops and laptops.
>
>   All PCIe ASPM are not enabled by BIOS, and those systems immediately
>   freeze once ASPM is enabled.
>
> These system hangs might be caused by (1) some hardware issue that
> causes a hang when ASPM is enabled even if it is configured correctly
> or (2) Linux configuring ASPM incorrectly.

Could this be:
1044 PCIe ® Controller May Hang on Entry Into Either L1.1 or
L1.2 Power Management Substate

Description
Under a highly specific and detailed set of internal timing
conditions, the PCIe ® controller may hang on entry
into either L1.1 or L1.2 power management substate.
This failure occurs when L1 power management substate exit is
triggered by a link partner asserting CLKREQ#
prior to the completion of the L1 power management stubstates entry protocol.

Potential Effect on System
The system may hang or reset.

Suggested Workaround
Disable L1.1 and L1.2 power management substates. System software may
contain the workaround for this
erratum.

Fix Planned
Yes

Link: https://www.amd.com/system/files/TechDocs/55449_Fam_17h_M_00h-0Fh_Rev_Guide.pdf

> For case (1), the platform should be using ACPI_FADT_NO_ASPM or _OSC
> to prevent the OS from enabling ASPM.  Linux should pay attention to
> that even when CONFIG_PCIEASPM_POWERSAVE=y.
>
> If the platform *should* use these mechanisms but doesn't, the
> solution is a quirk, not the folklore that "we can't use
> CONFIG_PCIEASPM_POWERSAVE=y because it breaks some systems."
>
> For case (2), we should fix Linux so it configures ASPM correctly.
>
> We cannot use the build-time CONFIG_PCIEASPM settings to avoid these
> hangs.  We need to fix the Linux run-time code so the system operates
> correctly no matter what CONFIG_PCIEASPM setting is used.
>
> We have sysfs knobs to control ASPM (see 72ea91afbfb0 ("PCI/ASPM: Add
> sysfs attributes for controlling ASPM link states")).  They can do the
> same thing at run-time as CONFIG_PCIEASPM_POWERSAVE=y does at
> build-time.  If those knobs cause hangs on 1st Gen Ryzen systems, we
> need to fix that.
>
> Bjorn

Re: [PATCH 2/2] PCI: vmd: Enable ASPM for mobile platforms

[Kai-Heng Feng]

> On Oct 7, 2020, at 21:30, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Wed, Oct 07, 2020 at 12:26:19PM +0800, Kai-Heng Feng wrote:
>>> On Oct 6, 2020, at 03:19, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Tue, Oct 06, 2020 at 02:40:32AM +0800, Kai-Heng Feng wrote:
>>>>> On Oct 3, 2020, at 06:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>> On Wed, Sep 30, 2020 at 04:24:54PM +0800, Kai-Heng Feng wrote:
> 
> ...
>>>> I wonder whether other devices that add PCIe domain have the same
>>>> behavior?  Maybe it's not a special case at all...
>>> 
>>> What other devices are these?
>> 
>> Controllers which add PCIe domain.
> 
> I was looking for specific examples, not just a restatement of what
> you said before.  I'm just curious because there are a lot of
> controllers I'm not familiar with, and I can't think of an example.
> 
>>>> I understand the end goal is to keep consistency for the entire ASPM
>>>> logic. However I can't think of any possible solution right now.
>>>> 
>>>>> - If we built with CONFIG_PCIEASPM_POWERSAVE=y, would that solve the
>>>>>  SoC power state problem?
>>>> 
>>>> Yes.
>>>> 
>>>>> - What issues would CONFIG_PCIEASPM_POWERSAVE=y introduce?
>>>> 
>>>> This will break many systems, at least for the 1st Gen Ryzen
>>>> desktops and laptops.
>>>> 
>>>> All PCIe ASPM are not enabled by BIOS, and those systems immediately
>>>> freeze once ASPM is enabled.
>>> 
>>> That indicates a defect in the Linux ASPM code.  We should fix that.
>>> It should be safe to use CONFIG_PCIEASPM_POWERSAVE=y on every system.
>> 
>> On those systems ASPM are also not enabled on Windows. So I think
>> ASPM are disabled for a reason.
> 
> If the platform knows ASPM needs to be disabled, it should be using
> ACPI_FADT_NO_ASPM or _OSC to prevent the OS from using it.  And if
> CONFIG_PCIEASPM_POWERSAVE=y means Linux enables ASPM when it
> shouldn't, that's a Linux bug that we need to fix.

Yes that's a bug which fixed by Ian's new patch.

> 
>>> Are there bug reports for these? The info we would need to start with
>>> includes "lspci -vv" and dmesg log (with CONFIG_PCIEASPM_DEFAULT=y).
>>> If a console log with CONFIG_PCIEASPM_POWERSAVE=y is available, that
>>> might be interesting, too.  We'll likely need to add some
>>> instrumentation and do some experimentation, but in principle, this
>>> should be fixable.
>> 
>> Doing this is asking users to use hardware settings that ODM/OEM
>> never tested, and I think the risk is really high.
> 
> What?  That's not what I said at all.  I'm asking for information
> about these hangs so we can fix them.  I'm not suggesting that you
> should switch to CONFIG_PCIEASPM_POWERSAVE=y for the distro.

Ah, I thought your suggestion is switching to CONFIG_PCIEASPM_POWERSAVE=y, because I sense you want to use that to cover the VMD ASPM this patch tries to solve.

Do we have a conclusion how to enable VMD ASPM with CONFIG_PCIEASPM_DEFAULT=y?

> 
> Let's back up.  You said:
> 
>  CONFIG_PCIEASPM_POWERSAVE=y ... will break many systems, at least
>  for the 1st Gen Ryzen desktops and laptops.
> 
>  All PCIe ASPM are not enabled by BIOS, and those systems immediately
>  freeze once ASPM is enabled.
> 
> These system hangs might be caused by (1) some hardware issue that
> causes a hang when ASPM is enabled even if it is configured correctly
> or (2) Linux configuring ASPM incorrectly.

It's (2) here.

> 
> For case (1), the platform should be using ACPI_FADT_NO_ASPM or _OSC
> to prevent the OS from enabling ASPM.  Linux should pay attention to
> that even when CONFIG_PCIEASPM_POWERSAVE=y.
> 
> If the platform *should* use these mechanisms but doesn't, the
> solution is a quirk, not the folklore that "we can't use
> CONFIG_PCIEASPM_POWERSAVE=y because it breaks some systems."

The platform in question doesn't prevent OS from enabling ASPM.

> 
> For case (2), we should fix Linux so it configures ASPM correctly.
> 
> We cannot use the build-time CONFIG_PCIEASPM settings to avoid these
> hangs.  We need to fix the Linux run-time code so the system operates
> correctly no matter what CONFIG_PCIEASPM setting is used.
> 
> We have sysfs knobs to control ASPM (see 72ea91afbfb0 ("PCI/ASPM: Add
> sysfs attributes for controlling ASPM link states")).  They can do the
> same thing at run-time as CONFIG_PCIEASPM_POWERSAVE=y does at
> build-time.  If those knobs cause hangs on 1st Gen Ryzen systems, we
> need to fix that.

Ian's patch solves the issue, at least for the systems I have.

Kai-Heng

> 
> Bjorn

Re: [PATCH 2/2] PCI: vmd: Enable ASPM for mobile platforms

[Ian Kumlien]

On Thu, Oct 8, 2020 at 6:19 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
>
>
> > On Oct 7, 2020, at 21:30, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Wed, Oct 07, 2020 at 12:26:19PM +0800, Kai-Heng Feng wrote:
> >>> On Oct 6, 2020, at 03:19, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>> On Tue, Oct 06, 2020 at 02:40:32AM +0800, Kai-Heng Feng wrote:
> >>>>> On Oct 3, 2020, at 06:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>>>> On Wed, Sep 30, 2020 at 04:24:54PM +0800, Kai-Heng Feng wrote:
> >
> > ...
> >>>> I wonder whether other devices that add PCIe domain have the same
> >>>> behavior?  Maybe it's not a special case at all...
> >>>
> >>> What other devices are these?
> >>
> >> Controllers which add PCIe domain.
> >
> > I was looking for specific examples, not just a restatement of what
> > you said before.  I'm just curious because there are a lot of
> > controllers I'm not familiar with, and I can't think of an example.
> >
> >>>> I understand the end goal is to keep consistency for the entire ASPM
> >>>> logic. However I can't think of any possible solution right now.
> >>>>
> >>>>> - If we built with CONFIG_PCIEASPM_POWERSAVE=y, would that solve the
> >>>>>  SoC power state problem?
> >>>>
> >>>> Yes.
> >>>>
> >>>>> - What issues would CONFIG_PCIEASPM_POWERSAVE=y introduce?
> >>>>
> >>>> This will break many systems, at least for the 1st Gen Ryzen
> >>>> desktops and laptops.
> >>>>
> >>>> All PCIe ASPM are not enabled by BIOS, and those systems immediately
> >>>> freeze once ASPM is enabled.
> >>>
> >>> That indicates a defect in the Linux ASPM code.  We should fix that.
> >>> It should be safe to use CONFIG_PCIEASPM_POWERSAVE=y on every system.
> >>
> >> On those systems ASPM are also not enabled on Windows. So I think
> >> ASPM are disabled for a reason.
> >
> > If the platform knows ASPM needs to be disabled, it should be using
> > ACPI_FADT_NO_ASPM or _OSC to prevent the OS from using it.  And if
> > CONFIG_PCIEASPM_POWERSAVE=y means Linux enables ASPM when it
> > shouldn't, that's a Linux bug that we need to fix.
>
> Yes that's a bug which fixed by Ian's new patch.
>
> >
> >>> Are there bug reports for these? The info we would need to start with
> >>> includes "lspci -vv" and dmesg log (with CONFIG_PCIEASPM_DEFAULT=y).
> >>> If a console log with CONFIG_PCIEASPM_POWERSAVE=y is available, that
> >>> might be interesting, too.  We'll likely need to add some
> >>> instrumentation and do some experimentation, but in principle, this
> >>> should be fixable.
> >>
> >> Doing this is asking users to use hardware settings that ODM/OEM
> >> never tested, and I think the risk is really high.
> >
> > What?  That's not what I said at all.  I'm asking for information
> > about these hangs so we can fix them.  I'm not suggesting that you
> > should switch to CONFIG_PCIEASPM_POWERSAVE=y for the distro.
>
> Ah, I thought your suggestion is switching to CONFIG_PCIEASPM_POWERSAVE=y, because I sense you want to use that to cover the VMD ASPM this patch tries to solve.
>
> Do we have a conclusion how to enable VMD ASPM with CONFIG_PCIEASPM_DEFAULT=y?
>
> >
> > Let's back up.  You said:
> >
> >  CONFIG_PCIEASPM_POWERSAVE=y ... will break many systems, at least
> >  for the 1st Gen Ryzen desktops and laptops.
> >
> >  All PCIe ASPM are not enabled by BIOS, and those systems immediately
> >  freeze once ASPM is enabled.
> >
> > These system hangs might be caused by (1) some hardware issue that
> > causes a hang when ASPM is enabled even if it is configured correctly
> > or (2) Linux configuring ASPM incorrectly.
>
> It's (2) here.
>
> >
> > For case (1), the platform should be using ACPI_FADT_NO_ASPM or _OSC
> > to prevent the OS from enabling ASPM.  Linux should pay attention to
> > that even when CONFIG_PCIEASPM_POWERSAVE=y.
> >
> > If the platform *should* use these mechanisms but doesn't, the
> > solution is a quirk, not the folklore that "we can't use
> > CONFIG_PCIEASPM_POWERSAVE=y because it breaks some systems."
>
> The platform in question doesn't prevent OS from enabling ASPM.
>
> >
> > For case (2), we should fix Linux so it configures ASPM correctly.
> >
> > We cannot use the build-time CONFIG_PCIEASPM settings to avoid these
> > hangs.  We need to fix the Linux run-time code so the system operates
> > correctly no matter what CONFIG_PCIEASPM setting is used.
> >
> > We have sysfs knobs to control ASPM (see 72ea91afbfb0 ("PCI/ASPM: Add
> > sysfs attributes for controlling ASPM link states")).  They can do the
> > same thing at run-time as CONFIG_PCIEASPM_POWERSAVE=y does at
> > build-time.  If those knobs cause hangs on 1st Gen Ryzen systems, we
> > need to fix that.
>
> Ian's patch solves the issue, at least for the systems I have.

Could you add:
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 15d64832a988..cd9f2101f9a2 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -482,7 +482,12 @@ static void pcie_aspm_check_latency(struct
pci_dev *endpoint)
                        latency = max_t(u32, link->latency_up.l1,
link->latency_dw.l1);
                        l1_max_latency = max_t(u32, latency, l1_max_latency);
                        if (l1_max_latency + l1_switch_latency > acceptable->l1)
+                       {
+                               pci_info(endpoint, "L1 latency
exceeded - path: %i - max: %i\n", l1_switch_latency, l1_max_latency);
+                               pci_info(link->pdev, "Upstream device
- %i\n", link->latency_up.l1);
+                               pci_info(link->downstream, "Downstream
device - %i\n", link->latency_dw.l1);
                                link->aspm_capable &= ~ASPM_STATE_L1;
+                       }

                        l1_switch_latency += 1000;
                }

So we can see what device triggers what links to be disabled?

I think your use-case is much more important than mine - mine fixes
something as a side effect

Also, please send me the lspci -vvv output as well as lspci -PP -s
<device id> for the device id:s mentioned in dmesg with the patch
above applied ;)

> Kai-Heng
>
> >
> > Bjorn
>