[PATCH] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[PATCH] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Rajat Jain]

Currently, the linux kernel disables ASPM when a device is
removed from the kernel. But it is not enabled again when
a new device is added on that slot even if it was originally
enabled (by the BIOS) when the system booted up (assuming
POLICY_DEFAULT).

This was earlier discussed here:
https://www.spinics.net/lists/linux-pci/msg60212.html

And some suggestions from Bjorn here:
https://www.spinics.net/lists/linux-pci/msg60541.html

This patch picks up one of the suggestion, to remove the
CONFIG_PCIEASPM_DEBUG and thus make the code always
avilable. This provides control to userspace to control
ASPM on a per slot / device basis using sysfs interface.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/pci/pci.h        | 5 -----
 drivers/pci/pcie/Kconfig | 8 --------
 drivers/pci/pcie/aspm.c  | 2 --
 3 files changed, 15 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 023f7cf25bff..383d92a6b0fb 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -365,13 +365,8 @@ 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
 
-#ifdef CONFIG_PCIEASPM_DEBUG
 void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
 void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
-#else
-static inline void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev) { }
-static inline void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev) { }
-#endif
 
 #ifdef CONFIG_PCIE_PTM
 void pci_ptm_init(struct pci_dev *dev);
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index b12e28b3d8f9..089b9f559d88 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -46,14 +46,6 @@ config PCIEASPM
 
 	  When in doubt, say Y.
 
-config PCIEASPM_DEBUG
-	bool "Debug PCI Express ASPM"
-	depends on PCIEASPM
-	default n
-	help
-	  This enables PCI Express ASPM debug support. It will add per-device
-	  interface to control ASPM.
-
 choice
 	prompt "Default ASPM policy"
 	default PCIEASPM_DEFAULT
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index c687c817b47d..8ffc13d42baa 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer, const struct kernel_param *kp)
 module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
 	NULL, 0644);
 
-#ifdef CONFIG_PCIEASPM_DEBUG
 static ssize_t link_state_show(struct device *dev,
 		struct device_attribute *attr,
 		char *buf)
@@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
 		sysfs_remove_file_from_group(&pdev->dev.kobj,
 			&dev_attr_clk_ctl.attr, power_group);
 }
-#endif
 
 static int __init pcie_aspm_disable(char *str)
 {
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[okaya]

On 2018-05-09 00:01, Rajat Jain wrote:
> Currently, the linux kernel disables ASPM when a device is
> removed from the kernel. But it is not enabled again when
> a new device is added on that slot even if it was originally
> enabled (by the BIOS) when the system booted up (assuming
> POLICY_DEFAULT).
> 
> This was earlier discussed here:
> https://www.spinics.net/lists/linux-pci/msg60212.html
> 
> And some suggestions from Bjorn here:
> https://www.spinics.net/lists/linux-pci/msg60541.html
> 
> This patch picks up one of the suggestion, to remove the
> CONFIG_PCIEASPM_DEBUG and thus make the code always
> avilable. This provides control to userspace to control
> ASPM on a per slot / device basis using sysfs interface.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>

Reviewed-by: Sinan Kaya <okaya@codeaurora.org>


> ---
>  drivers/pci/pci.h        | 5 -----
>  drivers/pci/pcie/Kconfig | 8 --------
>  drivers/pci/pcie/aspm.c  | 2 --
>  3 files changed, 15 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 023f7cf25bff..383d92a6b0fb 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -365,13 +365,8 @@ 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
> 
> -#ifdef CONFIG_PCIEASPM_DEBUG
>  void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
>  void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
> -#else
> -static inline void pcie_aspm_create_sysfs_dev_files(struct pci_dev 
> *pdev) { }
> -static inline void pcie_aspm_remove_sysfs_dev_files(struct pci_dev 
> *pdev) { }
> -#endif
> 
>  #ifdef CONFIG_PCIE_PTM
>  void pci_ptm_init(struct pci_dev *dev);
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index b12e28b3d8f9..089b9f559d88 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -46,14 +46,6 @@ config PCIEASPM
> 
>  	  When in doubt, say Y.
> 
> -config PCIEASPM_DEBUG
> -	bool "Debug PCI Express ASPM"
> -	depends on PCIEASPM
> -	default n
> -	help
> -	  This enables PCI Express ASPM debug support. It will add per-device
> -	  interface to control ASPM.
> -
>  choice
>  	prompt "Default ASPM policy"
>  	default PCIEASPM_DEFAULT
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index c687c817b47d..8ffc13d42baa 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer,
> const struct kernel_param *kp)
>  module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
>  	NULL, 0644);
> 
> -#ifdef CONFIG_PCIEASPM_DEBUG
>  static ssize_t link_state_show(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
> @@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct
> pci_dev *pdev)
>  		sysfs_remove_file_from_group(&pdev->dev.kobj,
>  			&dev_attr_clk_ctl.attr, power_group);
>  }
> -#endif
> 
>  static int __init pcie_aspm_disable(char *str)
>  {

Re: [PATCH] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

Hi Rajat,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pci/next]
[also build test ERROR on v4.17-rc4 next-20180508]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rajat-Jain/pci-aspm-Remove-CONFIG_PCIEASPM_DEBUG/20180509-161750
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-alldefconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/pci/pci-sysfs.o: In function `pci_create_sysfs_dev_files':
>> pci-sysfs.c:(.text+0x19b0): undefined reference to `pcie_aspm_create_sysfs_dev_files'
>> pci-sysfs.c:(.text+0x1a16): undefined reference to `pcie_aspm_remove_sysfs_dev_files'
   drivers/pci/pci-sysfs.o: In function `pci_remove_sysfs_dev_files':
   pci-sysfs.c:(.text+0x1af6): undefined reference to `pcie_aspm_remove_sysfs_dev_files'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10166 bytes --]

[PATCH] PM / s2idle: Clear the events_check_enabled flag

[Rajat Jain]

Problem: This flag does not get cleared currently in the suspend or
resume path in the following cases:

* In case some driver's suspend routine returns an error.
* Successful s2idle case
* etc?

Why is this a problem: What happens is that the next suspend attempt
could fail even though the user did not enable the flag by writing to
/sys/power/wakeup_count. This is 1 use case how the issue can be seen
(but similar use case with driver suspend failure can be thought of):

1. Read /sys/power/wakeup_count
2. echo count > /sys/power/wakeup_count
3. echo freeze > /sys/power/wakeup_count
4. Let the system suspend, and wakeup the system using some wake source
   that calls pm_wakeup_event() e.g. power button or something.
5. Note that the combined wakeup count would be incremented due
   to the pm_wakeup_event() in the resume path.
6. After resuming the events_check_enabled flag is still set.

At this point if the user attempts to freeze again (without writing to
/sys/power/wakeup_count), the suspend would fail even though there has
been no wake event since the past resume.

What this patch does:

It moves the clearing of the flag to just before a resume is completed,
so that it is always cleared for the corner cases mentioned above.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 kernel/power/suspend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index ccd2d20e6b06..0685c4499431 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -437,7 +437,6 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 			error = suspend_ops->enter(state);
 			trace_suspend_resume(TPS("machine_suspend"),
 				state, false);
-			events_check_enabled = false;
 		} else if (*wakeup) {
 			error = -EBUSY;
 		}
@@ -582,6 +581,7 @@ static int enter_state(suspend_state_t state)
 	pm_restore_gfp_mask();
 
  Finish:
+	events_check_enabled = false;
 	pm_pr_dbg("Finishing wakeup.\n");
 	suspend_finish();
  Unlock:
-- 
2.15.0.403.gc27cc4dac6-goog

Re: [PATCH] PM / s2idle: Clear the events_check_enabled flag

[Rajat Jain]

Sorry, please ignore, sent by mistake.

On Thu, May 10, 2018 at 4:34 PM, Rajat Jain <rajatja@google.com> wrote:
> Problem: This flag does not get cleared currently in the suspend or
> resume path in the following cases:
>
> * In case some driver's suspend routine returns an error.
> * Successful s2idle case
> * etc?
>
> Why is this a problem: What happens is that the next suspend attempt
> could fail even though the user did not enable the flag by writing to
> /sys/power/wakeup_count. This is 1 use case how the issue can be seen
> (but similar use case with driver suspend failure can be thought of):
>
> 1. Read /sys/power/wakeup_count
> 2. echo count > /sys/power/wakeup_count
> 3. echo freeze > /sys/power/wakeup_count
> 4. Let the system suspend, and wakeup the system using some wake source
>    that calls pm_wakeup_event() e.g. power button or something.
> 5. Note that the combined wakeup count would be incremented due
>    to the pm_wakeup_event() in the resume path.
> 6. After resuming the events_check_enabled flag is still set.
>
> At this point if the user attempts to freeze again (without writing to
> /sys/power/wakeup_count), the suspend would fail even though there has
> been no wake event since the past resume.
>
> What this patch does:
>
> It moves the clearing of the flag to just before a resume is completed,
> so that it is always cleared for the corner cases mentioned above.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  kernel/power/suspend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index ccd2d20e6b06..0685c4499431 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -437,7 +437,6 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
>                         error = suspend_ops->enter(state);
>                         trace_suspend_resume(TPS("machine_suspend"),
>                                 state, false);
> -                       events_check_enabled = false;
>                 } else if (*wakeup) {
>                         error = -EBUSY;
>                 }
> @@ -582,6 +581,7 @@ static int enter_state(suspend_state_t state)
>         pm_restore_gfp_mask();
>
>   Finish:
> +       events_check_enabled = false;
>         pm_pr_dbg("Finishing wakeup.\n");
>         suspend_finish();
>   Unlock:
> --
> 2.15.0.403.gc27cc4dac6-goog
>

[PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Rajat Jain]

Currently, the linux kernel disables ASPM when a device is
removed from the kernel. But it is not enabled again when
a new device is added on that slot even if it was originally
enabled (by the BIOS) when the system booted up (assuming
POLICY_DEFAULT).

This was earlier discussed here:
https://www.spinics.net/lists/linux-pci/msg60212.html

And some suggestions from Bjorn here:
https://www.spinics.net/lists/linux-pci/msg60541.html

This patch picks up one of the suggestion, to remove the
CONFIG_PCIEASPM_DEBUG and thus make the code always
avilable. This provides control to userspace to control
ASPM on a per slot / device basis using sysfs interface.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: provide function definitions for !CONFIG_PCIEASPM case

 drivers/pci/pci.h        | 8 ++------
 drivers/pci/pcie/Kconfig | 8 --------
 drivers/pci/pcie/aspm.c  | 2 --
 3 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 023f7cf25bff..b953b2349ca1 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -358,17 +358,13 @@ 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);
+void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
+void pcie_aspm_remove_sysfs_dev_files(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
-
-#ifdef CONFIG_PCIEASPM_DEBUG
-void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
-void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
-#else
 static inline void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev) { }
 static inline void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev) { }
 #endif
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index b12e28b3d8f9..089b9f559d88 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -46,14 +46,6 @@ config PCIEASPM
 
 	  When in doubt, say Y.
 
-config PCIEASPM_DEBUG
-	bool "Debug PCI Express ASPM"
-	depends on PCIEASPM
-	default n
-	help
-	  This enables PCI Express ASPM debug support. It will add per-device
-	  interface to control ASPM.
-
 choice
 	prompt "Default ASPM policy"
 	default PCIEASPM_DEFAULT
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index c687c817b47d..8ffc13d42baa 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer, const struct kernel_param *kp)
 module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
 	NULL, 0644);
 
-#ifdef CONFIG_PCIEASPM_DEBUG
 static ssize_t link_state_show(struct device *dev,
 		struct device_attribute *attr,
 		char *buf)
@@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
 		sysfs_remove_file_from_group(&pdev->dev.kobj,
 			&dev_attr_clk_ctl.attr, power_group);
 }
-#endif
 
 static int __init pcie_aspm_disable(char *str)
 {
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Rajat Jain]

Hello Bjorn,

Just checking to see if this one fell through the cracks? It isn't
solving any issues, but providing a tool for ASPM tweaking in user
space, which may be useful.

Thanks,

Rajat
On Thu, May 10, 2018 at 4:39 PM Rajat Jain <rajatja@google.com> wrote:
>
> Currently, the linux kernel disables ASPM when a device is
> removed from the kernel. But it is not enabled again when
> a new device is added on that slot even if it was originally
> enabled (by the BIOS) when the system booted up (assuming
> POLICY_DEFAULT).
>
> This was earlier discussed here:
> https://www.spinics.net/lists/linux-pci/msg60212.html
>
> And some suggestions from Bjorn here:
> https://www.spinics.net/lists/linux-pci/msg60541.html
>
> This patch picks up one of the suggestion, to remove the
> CONFIG_PCIEASPM_DEBUG and thus make the code always
> avilable. This provides control to userspace to control
> ASPM on a per slot / device basis using sysfs interface.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v2: provide function definitions for !CONFIG_PCIEASPM case
>
>  drivers/pci/pci.h        | 8 ++------
>  drivers/pci/pcie/Kconfig | 8 --------
>  drivers/pci/pcie/aspm.c  | 2 --
>  3 files changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 023f7cf25bff..b953b2349ca1 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -358,17 +358,13 @@ 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);
> +void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
> +void pcie_aspm_remove_sysfs_dev_files(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
> -
> -#ifdef CONFIG_PCIEASPM_DEBUG
> -void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
> -void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
> -#else
>  static inline void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev) { }
>  #endif
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index b12e28b3d8f9..089b9f559d88 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -46,14 +46,6 @@ config PCIEASPM
>
>           When in doubt, say Y.
>
> -config PCIEASPM_DEBUG
> -       bool "Debug PCI Express ASPM"
> -       depends on PCIEASPM
> -       default n
> -       help
> -         This enables PCI Express ASPM debug support. It will add per-device
> -         interface to control ASPM.
> -
>  choice
>         prompt "Default ASPM policy"
>         default PCIEASPM_DEFAULT
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index c687c817b47d..8ffc13d42baa 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer, const struct kernel_param *kp)
>  module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
>         NULL, 0644);
>
> -#ifdef CONFIG_PCIEASPM_DEBUG
>  static ssize_t link_state_show(struct device *dev,
>                 struct device_attribute *attr,
>                 char *buf)
> @@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
>                 sysfs_remove_file_from_group(&pdev->dev.kobj,
>                         &dev_attr_clk_ctl.attr, power_group);
>  }
> -#endif
>
>  static int __init pcie_aspm_disable(char *str)
>  {
> --
> 2.17.0.441.gb46fe60e1d-goog
>

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Sinan Kaya]

> On Thu, May 10, 2018 at 4:39 PM Rajat Jain <rajatja@google.com> wrote:
>>
>> Currently, the linux kernel disables ASPM when a device is
>> removed from the kernel. But it is not enabled again when
>> a new device is added on that slot even if it was originally
>> enabled (by the BIOS) when the system booted up (assuming
>> POLICY_DEFAULT).
>>
>> This was earlier discussed here:
>> https://www.spinics.net/lists/linux-pci/msg60212.html
>>
>> And some suggestions from Bjorn here:
>> https://www.spinics.net/lists/linux-pci/msg60541.html
>>
>> This patch picks up one of the suggestion, to remove the
>> CONFIG_PCIEASPM_DEBUG and thus make the code always
>> avilable. This provides control to userspace to control
>> ASPM on a per slot / device basis using sysfs interface.
>>
>> Signed-off-by: Rajat Jain <rajatja@google.com>
>> ---
>> v2: provide function definitions for !CONFIG_PCIEASPM case

Reviewed-by: Sinan Kaya <okaya@codeaurora.org>

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Bjorn Helgaas]

On Thu, May 10, 2018 at 04:39:12PM -0700, Rajat Jain wrote:
> Currently, the linux kernel disables ASPM when a device is
> removed from the kernel. But it is not enabled again when
> a new device is added on that slot even if it was originally
> enabled (by the BIOS) when the system booted up (assuming
> POLICY_DEFAULT).
> 
> This was earlier discussed here:
> https://www.spinics.net/lists/linux-pci/msg60212.html
> 
> And some suggestions from Bjorn here:
> https://www.spinics.net/lists/linux-pci/msg60541.html
> 
> This patch picks up one of the suggestion, to remove the
> CONFIG_PCIEASPM_DEBUG and thus make the code always
> avilable. This provides control to userspace to control
> ASPM on a per slot / device basis using sysfs interface.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>

Applied with Sinan's reviewed-by to pci/aspm for v4.19, thanks!

> ---
> v2: provide function definitions for !CONFIG_PCIEASPM case
> 
>  drivers/pci/pci.h        | 8 ++------
>  drivers/pci/pcie/Kconfig | 8 --------
>  drivers/pci/pcie/aspm.c  | 2 --
>  3 files changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 023f7cf25bff..b953b2349ca1 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -358,17 +358,13 @@ 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);
> +void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
> +void pcie_aspm_remove_sysfs_dev_files(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
> -
> -#ifdef CONFIG_PCIEASPM_DEBUG
> -void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
> -void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
> -#else
>  static inline void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev) { }
>  #endif
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index b12e28b3d8f9..089b9f559d88 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -46,14 +46,6 @@ config PCIEASPM
>  
>  	  When in doubt, say Y.
>  
> -config PCIEASPM_DEBUG
> -	bool "Debug PCI Express ASPM"
> -	depends on PCIEASPM
> -	default n
> -	help
> -	  This enables PCI Express ASPM debug support. It will add per-device
> -	  interface to control ASPM.
> -
>  choice
>  	prompt "Default ASPM policy"
>  	default PCIEASPM_DEFAULT
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index c687c817b47d..8ffc13d42baa 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer, const struct kernel_param *kp)
>  module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
>  	NULL, 0644);
>  
> -#ifdef CONFIG_PCIEASPM_DEBUG
>  static ssize_t link_state_show(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
> @@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
>  		sysfs_remove_file_from_group(&pdev->dev.kobj,
>  			&dev_attr_clk_ctl.attr, power_group);
>  }
> -#endif
>  
>  static int __init pcie_aspm_disable(char *str)
>  {
> -- 
> 2.17.0.441.gb46fe60e1d-goog
> 

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Bjorn Helgaas]

[+cc Rafael, Richard, Carlos, Pali, Takashi, Andy, Colin for question
about how to expose ASPM power management in sysfs]

On Thu, May 10, 2018 at 04:39:12PM -0700, Rajat Jain wrote:
> ...
> And some suggestions from Bjorn here:
> https://www.spinics.net/lists/linux-pci/msg60541.html
> 
> This patch picks up one of the suggestion, to remove the
> CONFIG_PCIEASPM_DEBUG and thus make the code always
> avilable. This provides control to userspace to control
> ASPM on a per slot / device basis using sysfs interface.

TL;DR: When CONFIG_PCIEASPM_DEBUG=y, Linux makes ASPM control files
       visible in sysfs, associated with the upstream end (e.g., Root
       Port) of a link.  Should these files be associated with the
       downstream end (e.g., NIC, GPU, etc) instead?

I think we do need to make ASPM control files visible in sysfs, as
this patch does.  But I have a question about exactly *how* we want to
do this.  I had planned to merge this patch for v4.19, but I think
I'll postpone it until we figure this out.

ASPM is a power management feature of a PCIe link, and it affects the
devices on both ends of that link.  The upstream end (closest to the
CPU) is typically a Root Port, and the downstream end is typically an
Endpoint (e.g., a GPU, NIC, or NVMe device).  For example, my laptop
has these devices:

  00:1c.2 Intel Root Port to [bus 04]
  04:00.0 Intel 8260 Wireless NIC

There's a PCIe link between these two devices, and if both ends
support it, ASPM reduces power usage when the NIC is idle.  The
hardware reduces power usage automatically; the kernel only needs to
configure ASPM.

ASPM affects performance as well as power, so we have knobs to control
the tradeoff.  Starting with the big hammers, we currently have:

  - Kernel build-time setting (CONFIG_PCIEASPM_PERFORMANCE, etc).
    Requires kernel rebuild and reboot.

  - "pcie_aspm=X" kernel boot parameter.  Can only enable/disable and
    requires a reboot.

  - /sys/module/pcie_aspm/parameters/policy file.  At any time, root
    can set the system-wide ASPM policy to one of these settings:

      + highest performance, highest power consumption
      + moderate power saving at cost of some performance
      + aggressive power saving at cost of more performance
      + use whatever setting BIOS configured

  - Many /sys/devices/pci0000:00/0000:00:1c.2/power/link_state files.
    At any time, root can set the ASPM policy to one of the above
    settings, but for individual devices.  Currently these files are
    only available when CONFIG_PCIEASPM_DEBUG=y (Red Hat does not
    enable this, but Ubuntu does).

The patch below changes it so these /sys/devices/.../link_state files
*always* exist, regardless of CONFIG_PCIEASPM_DEBUG.

The question is where those sysfs files should be.  Currently they are
associated with the device at the *upstream* end of the link.  In the
example above, they're associated with the Root Port:

  /sys/devices/pci0000:00/0000:00:1c.2/power/link_state

I don't know if that's the right place, or if they should be
associated with the device at the *downstream* end of the link, i.e.,
04:00.0.  The downstream end might be better because:

  - It's easier to associate the downstream end with a device the user
    cares about, e.g., a NIC, GPU, etc.  This is mostly a user-
    interface issue.

  - A link can lead to a multi-function device, and the spec allows
    those functions to have different ASPM settings (see PCIe r4.0,
    sec 5.4.1).  With the sysfs files at the upstream end of the link,
    we have no way to configure those functions individually.

Any thoughts?

> ...
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index b12e28b3d8f9..089b9f559d88 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -46,14 +46,6 @@ config PCIEASPM
>  
>  	  When in doubt, say Y.
>  
> -config PCIEASPM_DEBUG
> -	bool "Debug PCI Express ASPM"
> -	depends on PCIEASPM
> -	default n
> -	help
> -	  This enables PCI Express ASPM debug support. It will add per-device
> -	  interface to control ASPM.
> -
>  choice
>  	prompt "Default ASPM policy"
>  	default PCIEASPM_DEFAULT
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index c687c817b47d..8ffc13d42baa 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer, const struct kernel_param *kp)
>  module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
>  	NULL, 0644);
>  
> -#ifdef CONFIG_PCIEASPM_DEBUG
>  static ssize_t link_state_show(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
> @@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
>  		sysfs_remove_file_from_group(&pdev->dev.kobj,
>  			&dev_attr_clk_ctl.attr, power_group);
>  }
> -#endif
>  
>  static int __init pcie_aspm_disable(char *str)
>  {
> -- 
> 2.17.0.441.gb46fe60e1d-goog
> 

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Takashi Iwai]

On Fri, 27 Jul 2018 22:26:19 +0200,
Bjorn Helgaas wrote:
> 
> [+cc Rafael, Richard, Carlos, Pali, Takashi, Andy, Colin for question
> about how to expose ASPM power management in sysfs]
> 
> On Thu, May 10, 2018 at 04:39:12PM -0700, Rajat Jain wrote:
> > ...
> > And some suggestions from Bjorn here:
> > https://www.spinics.net/lists/linux-pci/msg60541.html
> > 
> > This patch picks up one of the suggestion, to remove the
> > CONFIG_PCIEASPM_DEBUG and thus make the code always
> > avilable. This provides control to userspace to control
> > ASPM on a per slot / device basis using sysfs interface.
> 
> TL;DR: When CONFIG_PCIEASPM_DEBUG=y, Linux makes ASPM control files
>        visible in sysfs, associated with the upstream end (e.g., Root
>        Port) of a link.  Should these files be associated with the
>        downstream end (e.g., NIC, GPU, etc) instead?
> 
> I think we do need to make ASPM control files visible in sysfs, as
> this patch does.  But I have a question about exactly *how* we want to
> do this.  I had planned to merge this patch for v4.19, but I think
> I'll postpone it until we figure this out.
> 
> ASPM is a power management feature of a PCIe link, and it affects the
> devices on both ends of that link.  The upstream end (closest to the
> CPU) is typically a Root Port, and the downstream end is typically an
> Endpoint (e.g., a GPU, NIC, or NVMe device).  For example, my laptop
> has these devices:
> 
>   00:1c.2 Intel Root Port to [bus 04]
>   04:00.0 Intel 8260 Wireless NIC
> 
> There's a PCIe link between these two devices, and if both ends
> support it, ASPM reduces power usage when the NIC is idle.  The
> hardware reduces power usage automatically; the kernel only needs to
> configure ASPM.
> 
> ASPM affects performance as well as power, so we have knobs to control
> the tradeoff.  Starting with the big hammers, we currently have:
> 
>   - Kernel build-time setting (CONFIG_PCIEASPM_PERFORMANCE, etc).
>     Requires kernel rebuild and reboot.
> 
>   - "pcie_aspm=X" kernel boot parameter.  Can only enable/disable and
>     requires a reboot.
> 
>   - /sys/module/pcie_aspm/parameters/policy file.  At any time, root
>     can set the system-wide ASPM policy to one of these settings:
> 
>       + highest performance, highest power consumption
>       + moderate power saving at cost of some performance
>       + aggressive power saving at cost of more performance
>       + use whatever setting BIOS configured
> 
>   - Many /sys/devices/pci0000:00/0000:00:1c.2/power/link_state files.
>     At any time, root can set the ASPM policy to one of the above
>     settings, but for individual devices.  Currently these files are
>     only available when CONFIG_PCIEASPM_DEBUG=y (Red Hat does not
>     enable this, but Ubuntu does).
> 
> The patch below changes it so these /sys/devices/.../link_state files
> *always* exist, regardless of CONFIG_PCIEASPM_DEBUG.
> 
> The question is where those sysfs files should be.  Currently they are
> associated with the device at the *upstream* end of the link.  In the
> example above, they're associated with the Root Port:
> 
>   /sys/devices/pci0000:00/0000:00:1c.2/power/link_state
> 
> I don't know if that's the right place, or if they should be
> associated with the device at the *downstream* end of the link, i.e.,
> 04:00.0.  The downstream end might be better because:
> 
>   - It's easier to associate the downstream end with a device the user
>     cares about, e.g., a NIC, GPU, etc.  This is mostly a user-
>     interface issue.
> 
>   - A link can lead to a multi-function device, and the spec allows
>     those functions to have different ASPM settings (see PCIe r4.0,
>     sec 5.4.1).  With the sysfs files at the upstream end of the link,
>     we have no way to configure those functions individually.
> 
> Any thoughts?

Through your description, having a control in the downstream side
sounds convincing enough.  The only drawback I can think of is the
complication of setups; you'd need to adjust each downstream node.

Maybe one option would be to add a new policy "let downstream decide",
and the downstream sysfs has effect only when the upstream sets this
mode.  When the upstream sets to another (currently existing) mode, it
forces the mode to all downstream devices.


Just my quick $0.02.


thanks,

Takashi

> 
> > ...
> > diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> > index b12e28b3d8f9..089b9f559d88 100644
> > --- a/drivers/pci/pcie/Kconfig
> > +++ b/drivers/pci/pcie/Kconfig
> > @@ -46,14 +46,6 @@ config PCIEASPM
> >  
> >  	  When in doubt, say Y.
> >  
> > -config PCIEASPM_DEBUG
> > -	bool "Debug PCI Express ASPM"
> > -	depends on PCIEASPM
> > -	default n
> > -	help
> > -	  This enables PCI Express ASPM debug support. It will add per-device
> > -	  interface to control ASPM.
> > -
> >  choice
> >  	prompt "Default ASPM policy"
> >  	default PCIEASPM_DEFAULT
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index c687c817b47d..8ffc13d42baa 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer, const struct kernel_param *kp)
> >  module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
> >  	NULL, 0644);
> >  
> > -#ifdef CONFIG_PCIEASPM_DEBUG
> >  static ssize_t link_state_show(struct device *dev,
> >  		struct device_attribute *attr,
> >  		char *buf)
> > @@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
> >  		sysfs_remove_file_from_group(&pdev->dev.kobj,
> >  			&dev_attr_clk_ctl.attr, power_group);
> >  }
> > -#endif
> >  
> >  static int __init pcie_aspm_disable(char *str)
> >  {
> > -- 
> > 2.17.0.441.gb46fe60e1d-goog
> > 
> 

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Sinan Kaya]

On 7/27/2018 1:26 PM, Bjorn Helgaas wrote:
> - A link can lead to a multi-function device, and the spec allows
>      those functions to have different ASPM settings (see PCIe r4.0,
>      sec 5.4.1).  With the sysfs files at the upstream end of the link,
>      we have no way to configure those functions individually.

Even though we can set them individually, there is only one PCIe link
for single function as well as multi-function devices.

It would be a user mistake to allow individual PCIe functions with
different ASPM policies. (maybe, we should enforce it if we are not
doing it already).

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Lukas Wunner]

On Fri, Jul 27, 2018 at 03:26:19PM -0500, Bjorn Helgaas wrote:
> The question is where those sysfs files should be.  Currently they are
> associated with the device at the *upstream* end of the link.  In the
> example above, they're associated with the Root Port:
> 
>   /sys/devices/pci0000:00/0000:00:1c.2/power/link_state
> 
> I don't know if that's the right place, or if they should be
> associated with the device at the *downstream* end of the link, i.e.,
> 04:00.0.  The downstream end might be better because:
> 
>   - It's easier to associate the downstream end with a device the user
>     cares about, e.g., a NIC, GPU, etc.  This is mostly a user-
>     interface issue.
> 
>   - A link can lead to a multi-function device, and the spec allows
>     those functions to have different ASPM settings (see PCIe r4.0,
>     sec 5.4.1).  With the sysfs files at the upstream end of the link,
>     we have no way to configure those functions individually.
> 
> Any thoughts?

Conceivably, could the downstream end not be enumerated at all?
(E.g. a hotplug or rescan is necessary for it to be enumerated?)

If so, the upstream end might be better because the ASPM settings
can be configured in advance, before the downstream end appears.

Thanks,

Lukas

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Bjorn Helgaas]

On Sat, Jul 28, 2018 at 05:16:13PM -0700, Sinan Kaya wrote:
> On 7/27/2018 1:26 PM, Bjorn Helgaas wrote:
> > - A link can lead to a multi-function device, and the spec allows
> >      those functions to have different ASPM settings (see PCIe r4.0,
> >      sec 5.4.1).  With the sysfs files at the upstream end of the link,
> >      we have no way to configure those functions individually.
> 
> Even though we can set them individually, there is only one PCIe link
> for single function as well as multi-function devices.
> 
> It would be a user mistake to allow individual PCIe functions with
> different ASPM policies. (maybe, we should enforce it if we are not
> doing it already).

It's true that multi-function devices share a single PCIe link.

However, the end of sec 5.4.1 does make it clear that the functions
need not have the same ASPM configuration, and it gives rules for how
those different settings should affect the shared link.  Since it
mentions different ASPM Control fields for the different functions, I
assume the policy combining those per-function settings into the
single link behavior must be implemented in the hardware.

Obviously we don't support per-function ASPM settings today.  I don't
know whether there's any value in supporting them or not, but putting
the control files at the upstream end basically precludes us from ever
supporting them.

Bjorn

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Sinan Kaya]

On 7/30/2018 7:14 AM, Bjorn Helgaas wrote:
> However, the end of sec 5.4.1 does make it clear that the functions
> need not have the same ASPM configuration, and it gives rules for how
> those different settings should affect the shared link.  Since it
> mentions different ASPM Control fields for the different functions, I
> assume the policy combining those per-function settings into the
> single link behavior must be implemented in the hardware.

Very interesting. This is news for me.

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Rajat Jain]

On Fri, Jul 27, 2018 at 1:26 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Rafael, Richard, Carlos, Pali, Takashi, Andy, Colin for question
> about how to expose ASPM power management in sysfs]
>
> On Thu, May 10, 2018 at 04:39:12PM -0700, Rajat Jain wrote:
>> ...
>> And some suggestions from Bjorn here:
>> https://www.spinics.net/lists/linux-pci/msg60541.html
>>
>> This patch picks up one of the suggestion, to remove the
>> CONFIG_PCIEASPM_DEBUG and thus make the code always
>> avilable. This provides control to userspace to control
>> ASPM on a per slot / device basis using sysfs interface.
>
> TL;DR: When CONFIG_PCIEASPM_DEBUG=y, Linux makes ASPM control files
>        visible in sysfs, associated with the upstream end (e.g., Root
>        Port) of a link.  Should these files be associated with the
>        downstream end (e.g., NIC, GPU, etc) instead?
>
> I think we do need to make ASPM control files visible in sysfs, as
> this patch does.  But I have a question about exactly *how* we want to
> do this.  I had planned to merge this patch for v4.19, but I think
> I'll postpone it until we figure this out.


Hi Bjorn,

Just a side note FYI, it is OK if you want to drop this, because we
anyway have this today as a config option so anyone who wants to use
these files can enable that config anyway.

Thanks,

Rajat

>
> ASPM is a power management feature of a PCIe link, and it affects the
> devices on both ends of that link.  The upstream end (closest to the
> CPU) is typically a Root Port, and the downstream end is typically an
> Endpoint (e.g., a GPU, NIC, or NVMe device).  For example, my laptop
> has these devices:
>
>   00:1c.2 Intel Root Port to [bus 04]
>   04:00.0 Intel 8260 Wireless NIC
>
> There's a PCIe link between these two devices, and if both ends
> support it, ASPM reduces power usage when the NIC is idle.  The
> hardware reduces power usage automatically; the kernel only needs to
> configure ASPM.
>
> ASPM affects performance as well as power, so we have knobs to control
> the tradeoff.  Starting with the big hammers, we currently have:
>
>   - Kernel build-time setting (CONFIG_PCIEASPM_PERFORMANCE, etc).
>     Requires kernel rebuild and reboot.
>
>   - "pcie_aspm=X" kernel boot parameter.  Can only enable/disable and
>     requires a reboot.
>
>   - /sys/module/pcie_aspm/parameters/policy file.  At any time, root
>     can set the system-wide ASPM policy to one of these settings:
>
>       + highest performance, highest power consumption
>       + moderate power saving at cost of some performance
>       + aggressive power saving at cost of more performance
>       + use whatever setting BIOS configured
>
>   - Many /sys/devices/pci0000:00/0000:00:1c.2/power/link_state files.
>     At any time, root can set the ASPM policy to one of the above
>     settings, but for individual devices.  Currently these files are
>     only available when CONFIG_PCIEASPM_DEBUG=y (Red Hat does not
>     enable this, but Ubuntu does).
>
> The patch below changes it so these /sys/devices/.../link_state files
> *always* exist, regardless of CONFIG_PCIEASPM_DEBUG.
>
> The question is where those sysfs files should be.  Currently they are
> associated with the device at the *upstream* end of the link.  In the
> example above, they're associated with the Root Port:
>
>   /sys/devices/pci0000:00/0000:00:1c.2/power/link_state
>
> I don't know if that's the right place, or if they should be
> associated with the device at the *downstream* end of the link, i.e.,
> 04:00.0.  The downstream end might be better because:
>
>   - It's easier to associate the downstream end with a device the user
>     cares about, e.g., a NIC, GPU, etc.  This is mostly a user-
>     interface issue.
>
>   - A link can lead to a multi-function device, and the spec allows
>     those functions to have different ASPM settings (see PCIe r4.0,
>     sec 5.4.1).  With the sysfs files at the upstream end of the link,
>     we have no way to configure those functions individually.
>
> Any thoughts?
>
>> ...
>> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
>> index b12e28b3d8f9..089b9f559d88 100644
>> --- a/drivers/pci/pcie/Kconfig
>> +++ b/drivers/pci/pcie/Kconfig
>> @@ -46,14 +46,6 @@ config PCIEASPM
>>
>>         When in doubt, say Y.
>>
>> -config PCIEASPM_DEBUG
>> -     bool "Debug PCI Express ASPM"
>> -     depends on PCIEASPM
>> -     default n
>> -     help
>> -       This enables PCI Express ASPM debug support. It will add per-device
>> -       interface to control ASPM.
>> -
>>  choice
>>       prompt "Default ASPM policy"
>>       default PCIEASPM_DEFAULT
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index c687c817b47d..8ffc13d42baa 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer, const struct kernel_param *kp)
>>  module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
>>       NULL, 0644);
>>
>> -#ifdef CONFIG_PCIEASPM_DEBUG
>>  static ssize_t link_state_show(struct device *dev,
>>               struct device_attribute *attr,
>>               char *buf)
>> @@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
>>               sysfs_remove_file_from_group(&pdev->dev.kobj,
>>                       &dev_attr_clk_ctl.attr, power_group);
>>  }
>> -#endif
>>
>>  static int __init pcie_aspm_disable(char *str)
>>  {
>> --
>> 2.17.0.441.gb46fe60e1d-goog
>>

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Bjorn Helgaas]

On Mon, Jul 30, 2018 at 10:32:10AM +0200, Lukas Wunner wrote:
> On Fri, Jul 27, 2018 at 03:26:19PM -0500, Bjorn Helgaas wrote:
> > The question is where those sysfs files should be.  Currently they are
> > associated with the device at the *upstream* end of the link.  In the
> > example above, they're associated with the Root Port:
> > 
> >   /sys/devices/pci0000:00/0000:00:1c.2/power/link_state
> > 
> > I don't know if that's the right place, or if they should be
> > associated with the device at the *downstream* end of the link, i.e.,
> > 04:00.0.  The downstream end might be better because:
> > 
> >   - It's easier to associate the downstream end with a device the user
> >     cares about, e.g., a NIC, GPU, etc.  This is mostly a user-
> >     interface issue.
> > 
> >   - A link can lead to a multi-function device, and the spec allows
> >     those functions to have different ASPM settings (see PCIe r4.0,
> >     sec 5.4.1).  With the sysfs files at the upstream end of the link,
> >     we have no way to configure those functions individually.
> > 
> > Any thoughts?
> 
> Conceivably, could the downstream end not be enumerated at all?
> (E.g. a hotplug or rescan is necessary for it to be enumerated?)
> 
> If so, the upstream end might be better because the ASPM settings
> can be configured in advance, before the downstream end appears.

Yes, of course there may be ports (root ports or switch downstream
ports) that have nothing connected yet.  The ASPM Control on a
hot-added device would normally be 0 (ASPM disabled).  Usually we can
enable features on the upstream end before the downstream end, but
this sentence in sec 5.4.1.3 is concerning:

  Software must not enable L0s in either direction on a given Link
  unless components on both sides of the Link each support L0s;
  otherwise, the result is undefined.

If we can trust that statement, if we enable ASPM on the upstream end,
then hot-add a device that doesn't support L0s, the result may be
undefined.  So I'm a little wary of configuring ASPM before a
downstream device is present.

I think the sysfs control files are absent if there's no downstream
device, but we create them when the hot-add happens, in paths like
this:

  pciehp_configure_device
    pci_scan_slot(bus)
      pcie_aspm_init_link_state(bus->self)

It seems ugly to me that this ASPM init is attached to the *parent*
instead of being done somewhere like pci_configure_device().

Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[Pali Rohár]

On Friday 27 July 2018 15:26:19 Bjorn Helgaas wrote:
> [+cc Rafael, Richard, Carlos, Pali, Takashi, Andy, Colin for question
> about how to expose ASPM power management in sysfs]
> 
> On Thu, May 10, 2018 at 04:39:12PM -0700, Rajat Jain wrote:
> > ...
> > And some suggestions from Bjorn here:
> > https://www.spinics.net/lists/linux-pci/msg60541.html
> > 
> > This patch picks up one of the suggestion, to remove the
> > CONFIG_PCIEASPM_DEBUG and thus make the code always
> > avilable. This provides control to userspace to control
> > ASPM on a per slot / device basis using sysfs interface.
> 
> TL;DR: When CONFIG_PCIEASPM_DEBUG=y, Linux makes ASPM control files
>        visible in sysfs, associated with the upstream end (e.g., Root
>        Port) of a link.  Should these files be associated with the
>        downstream end (e.g., NIC, GPU, etc) instead?
> 
> I think we do need to make ASPM control files visible in sysfs, as
> this patch does.  But I have a question about exactly *how* we want to
> do this.  I had planned to merge this patch for v4.19, but I think
> I'll postpone it until we figure this out.
> 
> ASPM is a power management feature of a PCIe link, and it affects the
> devices on both ends of that link.  The upstream end (closest to the
> CPU) is typically a Root Port, and the downstream end is typically an
> Endpoint (e.g., a GPU, NIC, or NVMe device).  For example, my laptop
> has these devices:
> 
>   00:1c.2 Intel Root Port to [bus 04]
>   04:00.0 Intel 8260 Wireless NIC
> 
> There's a PCIe link between these two devices, and if both ends
> support it, ASPM reduces power usage when the NIC is idle.  The
> hardware reduces power usage automatically; the kernel only needs to
> configure ASPM.
> 
> ASPM affects performance as well as power, so we have knobs to control
> the tradeoff.  Starting with the big hammers, we currently have:
> 
>   - Kernel build-time setting (CONFIG_PCIEASPM_PERFORMANCE, etc).
>     Requires kernel rebuild and reboot.
> 
>   - "pcie_aspm=X" kernel boot parameter.  Can only enable/disable and
>     requires a reboot.
> 
>   - /sys/module/pcie_aspm/parameters/policy file.  At any time, root
>     can set the system-wide ASPM policy to one of these settings:
> 
>       + highest performance, highest power consumption
>       + moderate power saving at cost of some performance
>       + aggressive power saving at cost of more performance
>       + use whatever setting BIOS configured
> 
>   - Many /sys/devices/pci0000:00/0000:00:1c.2/power/link_state files.
>     At any time, root can set the ASPM policy to one of the above
>     settings, but for individual devices.  Currently these files are
>     only available when CONFIG_PCIEASPM_DEBUG=y (Red Hat does not
>     enable this, but Ubuntu does).
> 
> The patch below changes it so these /sys/devices/.../link_state files
> *always* exist, regardless of CONFIG_PCIEASPM_DEBUG.
> 
> The question is where those sysfs files should be.  Currently they are
> associated with the device at the *upstream* end of the link.  In the
> example above, they're associated with the Root Port:
> 
>   /sys/devices/pci0000:00/0000:00:1c.2/power/link_state
> 
> I don't know if that's the right place, or if they should be
> associated with the device at the *downstream* end of the link, i.e.,
> 04:00.0.  The downstream end might be better because:
> 
>   - It's easier to associate the downstream end with a device the user
>     cares about, e.g., a NIC, GPU, etc.  This is mostly a user-
>     interface issue.
> 
>   - A link can lead to a multi-function device, and the spec allows
>     those functions to have different ASPM settings (see PCIe r4.0,
>     sec 5.4.1).  With the sysfs files at the upstream end of the link,
>     we have no way to configure those functions individually.
> 
> Any thoughts?

Hi Bjorn, in my opinion sysfs entries should at the downstream end. As I
think primary usage is to put "end" downstream device (e.g. wifi card)
into lower power mode to increase battery runtime on laptop.

Anyway, if sysfs entry is going to be moved from one place to another
maybe it would be better to give it a new name. Because currently it
sysfs entry is bound to the upstream device and if you change location,
then sysfs entry would change it logic.

-- 
Pali Rohár
pali.rohar@gmail.com