* [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()
@ 2022-05-09 7:36 Kai-Heng Feng
2022-06-21 2:27 ` Kai-Heng Feng
2022-07-11 23:11 ` Bjorn Helgaas
0 siblings, 2 replies; 3+ messages in thread
From: Kai-Heng Feng @ 2022-05-09 7:36 UTC (permalink / raw)
To: bhelgaas
Cc: Kai-Heng Feng, Saheed O. Bolarinwa, Krzysztof Wilczyński,
Logan Gunthorpe, Rajat Jain, linux-pci, linux-kernel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()
2022-05-09 7:36 [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change() Kai-Heng Feng
@ 2022-06-21 2:27 ` Kai-Heng Feng
2022-07-11 23:11 ` Bjorn Helgaas
1 sibling, 0 replies; 3+ messages in thread
From: Kai-Heng Feng @ 2022-06-21 2:27 UTC (permalink / raw)
To: bhelgaas
Cc: Saheed O. Bolarinwa, Krzysztof Wilczyński, Logan Gunthorpe,
Rajat Jain, linux-pci, linux-kernel
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change()
2022-05-09 7:36 [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change() Kai-Heng Feng
2022-06-21 2:27 ` Kai-Heng Feng
@ 2022-07-11 23:11 ` Bjorn Helgaas
1 sibling, 0 replies; 3+ messages in thread
From: Bjorn Helgaas @ 2022-07-11 23:11 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: bhelgaas, Saheed O. Bolarinwa, Krzysztof Wilczyński,
Logan Gunthorpe, Rajat Jain, linux-pci, linux-kernel
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-11 23:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-09 7:36 [PATCH] PCI:ASPM: Remove pcie_aspm_pm_state_change() Kai-Heng Feng
2022-06-21 2:27 ` Kai-Heng Feng
2022-07-11 23:11 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).