[PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

[PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

[Kai-Heng Feng]

pcie_aspm_pm_state_change() was introduced at the inception of PCIe
ASPM code.

However, it can cause some issues. For instance, when ASPM config is
changed via sysfs, those changes won't persist across power state change
because pcie_aspm_pm_state_change() overwrites them.

In addition to that, if the driver is to restore L1ss [1] after system
resume, the restored states will also be overwritten by
pcie_aspm_pm_state_change().

So remove pcie_aspm_pm_state_change() for now, if there's any hardware
really needs it to function, a quirk can be used instead.

[1] https://lore.kernel.org/linux-pci/20220201123536.12962-1-vidyas@nvidia.com/

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/pci.c       |  3 ---
 drivers/pci/pci.h       |  2 --
 drivers/pci/pcie/aspm.c | 19 -------------------
 3 files changed, 24 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ecce435fb3f1..d09f7b60ee4dc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1181,9 +1181,6 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (need_restore)
 		pci_restore_bars(dev);
 
-	if (dev->bus->self)
-		pcie_aspm_pm_state_change(dev->bus->self);
-
 	return 0;
 }
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3d60cabde1a15..86a19f293d4ad 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -560,12 +560,10 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
 #ifdef CONFIG_PCIEASPM
 void pcie_aspm_init_link_state(struct pci_dev *pdev);
 void pcie_aspm_exit_link_state(struct pci_dev *pdev);
-void pcie_aspm_pm_state_change(struct pci_dev *pdev);
 void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
 #else
 static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
 static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
-static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
 static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
 #endif
 
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a96b7424c9bc8..7f76a5875feb4 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1012,25 +1012,6 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 	up_read(&pci_bus_sem);
 }
 
-/* @pdev: the root port or switch downstream port */
-void pcie_aspm_pm_state_change(struct pci_dev *pdev)
-{
-	struct pcie_link_state *link = pdev->link_state;
-
-	if (aspm_disabled || !link)
-		return;
-	/*
-	 * Devices changed PM state, we should recheck if latency
-	 * meets all functions' requirement
-	 */
-	down_read(&pci_bus_sem);
-	mutex_lock(&aspm_lock);
-	pcie_update_aspm_capable(link->root);
-	pcie_config_aspm_path(link);
-	mutex_unlock(&aspm_lock);
-	up_read(&pci_bus_sem);
-}
-
 void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
 {
 	struct pcie_link_state *link = pdev->link_state;
-- 
2.34.1

Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

[Kai-Heng Feng]

On Mon, May 9, 2022 at 3:37 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> pcie_aspm_pm_state_change() was introduced at the inception of PCIe
> ASPM code.
>
> However, it can cause some issues. For instance, when ASPM config is
> changed via sysfs, those changes won't persist across power state change
> because pcie_aspm_pm_state_change() overwrites them.
>
> In addition to that, if the driver is to restore L1ss [1] after system
> resume, the restored states will also be overwritten by
> pcie_aspm_pm_state_change().
>
> So remove pcie_aspm_pm_state_change() for now, if there's any hardware
> really needs it to function, a quirk can be used instead.
>
> [1] https://lore.kernel.org/linux-pci/20220201123536.12962-1-vidyas@nvidia.com/
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

A gentle ping...

> ---
>  drivers/pci/pci.c       |  3 ---
>  drivers/pci/pci.h       |  2 --
>  drivers/pci/pcie/aspm.c | 19 -------------------
>  3 files changed, 24 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9ecce435fb3f1..d09f7b60ee4dc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1181,9 +1181,6 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
>         if (need_restore)
>                 pci_restore_bars(dev);
>
> -       if (dev->bus->self)
> -               pcie_aspm_pm_state_change(dev->bus->self);
> -
>         return 0;
>  }
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3d60cabde1a15..86a19f293d4ad 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -560,12 +560,10 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
>  #ifdef CONFIG_PCIEASPM
>  void pcie_aspm_init_link_state(struct pci_dev *pdev);
>  void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> -void pcie_aspm_pm_state_change(struct pci_dev *pdev);
>  void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
>  #else
>  static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
> -static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
>  #endif
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a96b7424c9bc8..7f76a5875feb4 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1012,25 +1012,6 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>         up_read(&pci_bus_sem);
>  }
>
> -/* @pdev: the root port or switch downstream port */
> -void pcie_aspm_pm_state_change(struct pci_dev *pdev)
> -{
> -       struct pcie_link_state *link = pdev->link_state;
> -
> -       if (aspm_disabled || !link)
> -               return;
> -       /*
> -        * Devices changed PM state, we should recheck if latency
> -        * meets all functions' requirement
> -        */
> -       down_read(&pci_bus_sem);
> -       mutex_lock(&aspm_lock);
> -       pcie_update_aspm_capable(link->root);
> -       pcie_config_aspm_path(link);
> -       mutex_unlock(&aspm_lock);
> -       up_read(&pci_bus_sem);
> -}
> -
>  void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
>  {
>         struct pcie_link_state *link = pdev->link_state;
> --
> 2.34.1
>

Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

[Bjorn Helgaas]

On Mon, May 09, 2022 at 03:36:37PM +0800, Kai-Heng Feng wrote:
> pcie_aspm_pm_state_change() was introduced at the inception of PCIe
> ASPM code.
> 
> However, it can cause some issues. For instance, when ASPM config is
> changed via sysfs, those changes won't persist across power state change
> because pcie_aspm_pm_state_change() overwrites them.
> 
> In addition to that, if the driver is to restore L1ss [1] after system
> resume, the restored states will also be overwritten by
> pcie_aspm_pm_state_change().
> 
> So remove pcie_aspm_pm_state_change() for now, if there's any hardware
> really needs it to function, a quirk can be used instead.
> 
> [1] https://lore.kernel.org/linux-pci/20220201123536.12962-1-vidyas@nvidia.com/
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Applied to pci/aspm for v5.20, thanks!

> ---
>  drivers/pci/pci.c       |  3 ---
>  drivers/pci/pci.h       |  2 --
>  drivers/pci/pcie/aspm.c | 19 -------------------
>  3 files changed, 24 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9ecce435fb3f1..d09f7b60ee4dc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1181,9 +1181,6 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
>  	if (need_restore)
>  		pci_restore_bars(dev);
>  
> -	if (dev->bus->self)
> -		pcie_aspm_pm_state_change(dev->bus->self);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3d60cabde1a15..86a19f293d4ad 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -560,12 +560,10 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
>  #ifdef CONFIG_PCIEASPM
>  void pcie_aspm_init_link_state(struct pci_dev *pdev);
>  void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> -void pcie_aspm_pm_state_change(struct pci_dev *pdev);
>  void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
>  #else
>  static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
> -static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
>  #endif
>  
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a96b7424c9bc8..7f76a5875feb4 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1012,25 +1012,6 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>  	up_read(&pci_bus_sem);
>  }
>  
> -/* @pdev: the root port or switch downstream port */
> -void pcie_aspm_pm_state_change(struct pci_dev *pdev)
> -{
> -	struct pcie_link_state *link = pdev->link_state;
> -
> -	if (aspm_disabled || !link)
> -		return;
> -	/*
> -	 * Devices changed PM state, we should recheck if latency
> -	 * meets all functions' requirement
> -	 */
> -	down_read(&pci_bus_sem);
> -	mutex_lock(&aspm_lock);
> -	pcie_update_aspm_capable(link->root);
> -	pcie_config_aspm_path(link);
> -	mutex_unlock(&aspm_lock);
> -	up_read(&pci_bus_sem);
> -}
> -
>  void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
>  {
>  	struct pcie_link_state *link = pdev->link_state;
> -- 
> 2.34.1
> 

Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

[Mark Hasemeyer]

> What is the git commit id of this change in Linus's tree?
08d0cc5f34265d1a1e3031f319f594bd1970976c

> And can you send it as a stand-alone patch, not one that I have to
> hand-edit out of an email to use?  Doing that does not scale at the rate
> of change we currently deal with at all.
Understood. I'll send a stand-alone patch.

Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

[Greg KH]

On Mon, Apr 24, 2023 at 12:35:36PM -0600, Mark Hasemeyer wrote:
> > Odd, it does not apply cleanly, so how was this tested?  Can you please
> > send the tested backport that you have so we know to get it correct?
> 
> Sorry about that. I had to apply a trivial backport as
> `pci_set_low_power_state` does not exist in v5.15.  It was tested by using an
> RTC wake in combination with using the sysfs to trigger a suspend:
> ```
> echo +5 > /sys/class/rtc/rtc0/wakealarm && echo freeze > /sys/power/state
> ```
> 
> Patch below.
> ------------------------------------
> >From 5ca368f6918710bf491feee54e09a060de835d3f Mon Sep 17 00:00:00 2001
> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Date: Mon, 11 Jul 2022 18:07:01 -0500
> Subject: [PATCH] PCI/ASPM: Remove pcie_aspm_pm_state_change()
> 
> pcie_aspm_pm_state_change() was introduced at the inception of PCIe ASPM
> code, but it can cause some issues. For instance, when ASPM config is
> changed via sysfs, those changes won't persist across power state change
> because pcie_aspm_pm_state_change() overwrites them.
> 
> Also, if the driver restores L1SS [1] after system resume, the restored
> state will also be overwritten by pcie_aspm_pm_state_change().
> 
> Remove pcie_aspm_pm_state_change().  If there's any hardware that really
> needs it to function, a quirk can be used instead.
> 
> [1] https://lore.kernel.org/linux-pci/20220201123536.12962-1-vidyas@nvidia.com/
> Link: https://lore.kernel.org/r/20220509073639.2048236-1-kai.heng.feng@canonical.com
> [bhelgaas: remove additional pcie_aspm_pm_state_change() call in
> pci_set_low_power_state(), added by
> 10aa5377fc8a ("PCI/PM: Split pci_raw_set_power_state()") and moved by
> 7957d201456f ("PCI/PM: Relocate pci_set_low_power_state()")]
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
>  drivers/pci/pci.c       |  3 ---
>  drivers/pci/pci.h       |  2 --
>  drivers/pci/pcie/aspm.c | 19 -------------------
>  3 files changed, 24 deletions(-)

What is the git commit id of this change in Linus's tree?

And can you send it as a stand-alone patch, not one that I have to
hand-edit out of an email to use?  Doing that does not scale at the rate
of change we currently deal with at all.

thanks,

greg k-h

Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

[Mark Hasemeyer]

> Odd, it does not apply cleanly, so how was this tested?  Can you please
> send the tested backport that you have so we know to get it correct?

Sorry about that. I had to apply a trivial backport as
`pci_set_low_power_state` does not exist in v5.15.  It was tested by using an
RTC wake in combination with using the sysfs to trigger a suspend:
```
echo +5 > /sys/class/rtc/rtc0/wakealarm && echo freeze > /sys/power/state
```

Patch below.
------------------------------------
From 5ca368f6918710bf491feee54e09a060de835d3f Mon Sep 17 00:00:00 2001
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
Date: Mon, 11 Jul 2022 18:07:01 -0500
Subject: [PATCH] PCI/ASPM: Remove pcie_aspm_pm_state_change()

pcie_aspm_pm_state_change() was introduced at the inception of PCIe ASPM
code, but it can cause some issues. For instance, when ASPM config is
changed via sysfs, those changes won't persist across power state change
because pcie_aspm_pm_state_change() overwrites them.

Also, if the driver restores L1SS [1] after system resume, the restored
state will also be overwritten by pcie_aspm_pm_state_change().

Remove pcie_aspm_pm_state_change().  If there's any hardware that really
needs it to function, a quirk can be used instead.

[1] https://lore.kernel.org/linux-pci/20220201123536.12962-1-vidyas@nvidia.com/
Link: https://lore.kernel.org/r/20220509073639.2048236-1-kai.heng.feng@canonical.com
[bhelgaas: remove additional pcie_aspm_pm_state_change() call in
pci_set_low_power_state(), added by
10aa5377fc8a ("PCI/PM: Split pci_raw_set_power_state()") and moved by
7957d201456f ("PCI/PM: Relocate pci_set_low_power_state()")]
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---
 drivers/pci/pci.c       |  3 ---
 drivers/pci/pci.h       |  2 --
 drivers/pci/pcie/aspm.c | 19 -------------------
 3 files changed, 24 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 649df298869c..4aa2e655398c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1140,9 +1140,6 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (need_restore)
 		pci_restore_bars(dev);
 
-	if (dev->bus->self)
-		pcie_aspm_pm_state_change(dev->bus->self);
-
 	return 0;
 }
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 72280e9b23b2..e6ea6e950428 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -595,12 +595,10 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active);
 #ifdef CONFIG_PCIEASPM
 void pcie_aspm_init_link_state(struct pci_dev *pdev);
 void pcie_aspm_exit_link_state(struct pci_dev *pdev);
-void pcie_aspm_pm_state_change(struct pci_dev *pdev);
 void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
 #else
 static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
 static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
-static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
 static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
 #endif
 
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 013a47f587ce..b3ad316418f1 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1020,25 +1020,6 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 	up_read(&pci_bus_sem);
 }
 
-/* @pdev: the root port or switch downstream port */
-void pcie_aspm_pm_state_change(struct pci_dev *pdev)
-{
-	struct pcie_link_state *link = pdev->link_state;
-
-	if (aspm_disabled || !link)
-		return;
-	/*
-	 * Devices changed PM state, we should recheck if latency
-	 * meets all functions' requirement
-	 */
-	down_read(&pci_bus_sem);
-	mutex_lock(&aspm_lock);
-	pcie_update_aspm_capable(link->root);
-	pcie_config_aspm_path(link);
-	mutex_unlock(&aspm_lock);
-	up_read(&pci_bus_sem);
-}
-
 void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
 {
 	struct pcie_link_state *link = pdev->link_state;
-- 
2.39.2

Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

[Greg KH]

On Fri, Apr 21, 2023 at 12:42:30PM -0600, Mark Hasemeyer wrote:
> commit 08d0cc5f34265d1a1e3031f319f594bd1970976c upstream.
> 
> This change is desired because without it, it has been observed that
> re-applying aspm settings can cause the system to crash with certain pci
> devices (ie. Genesys GL9755).
> 
> Tested by issuing 100 suspend/resume cycles on a symptomatic system running
> 5.15.107.
> 
> L1 settings looked identical before and after:
> ```
> localhost ~ # lspci -vvv -d 0x17a0: | grep L1Sub
>                 L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
>                 L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1+ ASPM_L1.2- ASPM_L1.1+
>                 L1SubCtl2: T_PwrOn=3100us
> ```
> 
> Cc: <stable@vger.kernel.org> # 5.15.y

Odd, it does not apply cleanly, so how was this tested?  Can you please
send the tested backport that you have so we know to get it correct?

thanks,

greg k-h

[PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

[Mark Hasemeyer]

commit 08d0cc5f34265d1a1e3031f319f594bd1970976c upstream.

This change is desired because without it, it has been observed that
re-applying aspm settings can cause the system to crash with certain pci
devices (ie. Genesys GL9755).

Tested by issuing 100 suspend/resume cycles on a symptomatic system running
5.15.107.

L1 settings looked identical before and after:
```
localhost ~ # lspci -vvv -d 0x17a0: | grep L1Sub
                L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
                L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1+ ASPM_L1.2- ASPM_L1.1+
                L1SubCtl2: T_PwrOn=3100us
```

Cc: <stable@vger.kernel.org> # 5.15.y

[PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()

[Mark Hasemeyer]

commit 08d0cc5f34265d1a1e3031f319f594bd1970976c upstream.

This change is desired because without it, it has been observed that
re-applying aspm settings can cause the system to crash with certain pci
devices (ie. Genesys GL9755).

Tested by issuing 100 suspend/resume cycles on a symptomatic system running
5.15.107.

L1 settings looked identical before and after:
```
localhost ~ # lspci -vvv -d 0x17a0: | grep L1Sub
                L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
                L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1+ ASPM_L1.2- ASPM_L1.1+
                L1SubCtl2: T_PwrOn=3100us
```

Cc: <stable@vger.kernel.org> # 5.15.y