[PATCH 1/2] PCI/ASPM: Store disabled ASPM states

[PATCH 1/2] PCI/ASPM: Store disabled ASPM states

[Kai-Heng Feng]

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

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

link# echo 0 > l1_aspm

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

link# echo 1 > l1_2_aspm

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

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

So store the disabled ASPM states for consistency.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/pcie/aspm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index ac0557a305af..2ea9fddadfad 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -658,6 +658,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
 
+	link->aspm_disable = link->aspm_capable & ~link->aspm_default;
+
 	/* Get and check endpoint acceptable latencies */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
 		u32 reg32, encoding;
@@ -1226,11 +1228,15 @@ static ssize_t aspm_attr_store_common(struct device *dev,
 	mutex_lock(&aspm_lock);
 
 	if (state_enable) {
-		link->aspm_disable &= ~state;
 		/* need to enable L1 for substates */
 		if (state & ASPM_STATE_L1SS)
-			link->aspm_disable &= ~ASPM_STATE_L1;
+			state |= ASPM_STATE_L1;
+
+		link->aspm_disable &= ~state;
 	} else {
+		if (state == ASPM_STATE_L1)
+			state |= ASPM_STATE_L1SS;
+
 		link->aspm_disable |= state;
 	}
 
-- 
2.29.2

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

[Kai-Heng Feng]

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

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

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/pci/pcie/aspm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 2ea9fddadfad..326da7bbc84d 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1239,8 +1239,7 @@ static ssize_t aspm_attr_store_common(struct device *dev,
 
 		link->aspm_disable |= state;
 	}
-
-	pcie_config_aspm_link(link, policy_to_aspm_state(link));
+	pcie_config_aspm_link(link, link->aspm_capable);
 
 	mutex_unlock(&aspm_lock);
 	up_read(&pci_bus_sem);
-- 
2.29.2

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

[Heiner Kallweit]

Am 08.12.2020 um 09:25 schrieb Kai-Heng Feng:
> If we are to use sysfs to change ASPM settings, we may want to override
> the default ASPM policy.
> 
> So use ASPM capability, instead of default policy, to be able to use all
> possible ASPM states.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/pci/pcie/aspm.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 2ea9fddadfad..326da7bbc84d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1239,8 +1239,7 @@ static ssize_t aspm_attr_store_common(struct device *dev,
>  
>  		link->aspm_disable |= state;
>  	}
> -
> -	pcie_config_aspm_link(link, policy_to_aspm_state(link));
> +	pcie_config_aspm_link(link, link->aspm_capable);
>  
I like the idea, because the policy can be changed by the user anyway.
Therefore I don't see it as a hard system limit.

However I think this change is not sufficient. Each call to
pcie_config_aspm_link(link, policy_to_aspm_state(link)), e.g. in path
pcie_aspm_pm_state_change -> pcie_config_aspm_path will reset the
enabled states to the policy.

>  	mutex_unlock(&aspm_lock);
>  	up_read(&pci_bus_sem);
> 

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

[Kai-Heng Feng]

> On Dec 9, 2020, at 01:18, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
> Am 08.12.2020 um 09:25 schrieb Kai-Heng Feng:
>> If we are to use sysfs to change ASPM settings, we may want to override
>> the default ASPM policy.
>> 
>> So use ASPM capability, instead of default policy, to be able to use all
>> possible ASPM states.
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/pci/pcie/aspm.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index 2ea9fddadfad..326da7bbc84d 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -1239,8 +1239,7 @@ static ssize_t aspm_attr_store_common(struct device *dev,
>> 
>> 		link->aspm_disable |= state;
>> 	}
>> -
>> -	pcie_config_aspm_link(link, policy_to_aspm_state(link));
>> +	pcie_config_aspm_link(link, link->aspm_capable);
>> 
> I like the idea, because the policy can be changed by the user anyway.
> Therefore I don't see it as a hard system limit.
> 
> However I think this change is not sufficient. Each call to
> pcie_config_aspm_link(link, policy_to_aspm_state(link)), e.g. in path
> pcie_aspm_pm_state_change -> pcie_config_aspm_path will reset the
> enabled states to the policy.

That's right, let me work this in v2.

Kai-Heng

> 
>> 	mutex_unlock(&aspm_lock);
>> 	up_read(&pci_bus_sem);
>> 
> 

Re: [PATCH 1/2] PCI/ASPM: Store disabled ASPM states

[Heiner Kallweit]

Am 08.12.2020 um 09:25 schrieb Kai-Heng Feng:
> If we use sysfs to disable L1 ASPM, then enable one L1 ASPM substate
> again, all other substates will also be enabled too:
> 
> link# grep . *
> clkpm:1
> l0s_aspm:1
> l1_1_aspm:1
> l1_1_pcipm:1
> l1_2_aspm:1
> l1_2_pcipm:1
> l1_aspm:1
> 
> link# echo 0 > l1_aspm
> 
> link# grep . *
> clkpm:1
> l0s_aspm:1
> l1_1_aspm:0
> l1_1_pcipm:0
> l1_2_aspm:0
> l1_2_pcipm:0
> l1_aspm:0
> 
> link# echo 1 > l1_2_aspm
> 
> link# grep . *
> clkpm:1
> l0s_aspm:1
> l1_1_aspm:1
> l1_1_pcipm:1
> l1_2_aspm:1
> l1_2_pcipm:1
> l1_aspm:1
> 
> This is because disabled ASPM states weren't saved, so enable any of the
> substate will also enable others.
> 
> So store the disabled ASPM states for consistency.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/pci/pcie/aspm.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index ac0557a305af..2ea9fddadfad 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -658,6 +658,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  	/* Setup initial capable state. Will be updated later */
>  	link->aspm_capable = link->aspm_support;
>  
> +	link->aspm_disable = link->aspm_capable & ~link->aspm_default;
> +

This makes sense only in combination with patch 2. However I think patch 1
should be independent of patch 2. Especially if we consider patch 1 a fix
that is applied to stable whilst patch 2 is an improvement for next.

>  	/* Get and check endpoint acceptable latencies */
>  	list_for_each_entry(child, &linkbus->devices, bus_list) {
>  		u32 reg32, encoding;
> @@ -1226,11 +1228,15 @@ static ssize_t aspm_attr_store_common(struct device *dev,
>  	mutex_lock(&aspm_lock);
>  
>  	if (state_enable) {
> -		link->aspm_disable &= ~state;
>  		/* need to enable L1 for substates */
>  		if (state & ASPM_STATE_L1SS)
> -			link->aspm_disable &= ~ASPM_STATE_L1;
> +			state |= ASPM_STATE_L1;
> +
> +		link->aspm_disable &= ~state;

I don't see what this part of the patch changes. Can you elaborate on why
this is needed?

>  	} else {
> +		if (state == ASPM_STATE_L1)
> +			state |= ASPM_STATE_L1SS;
> +

I think this part should be sufficient to fix the behavior. because what
I think currently happens:

1. original status: policy powersupersave, nothing disabled -> L1 + L1SS active
2. disable L1: L1 disabled, pcie_config_aspm_link() disabled L1 and L1SS
   w/o adding L1SS to link-> aspm_disabled
3. enable one L1SS state: aspm_attr_store_common() removes L1 from
   link->aspm_disabled -> link->aspm_disabled is empty, resulting in
   L1 + L1SS being active

>  		link->aspm_disable |= state;
>  	}
>  
> 

Re: [PATCH 1/2] PCI/ASPM: Store disabled ASPM states

[Kai-Heng Feng]

> On Dec 9, 2020, at 01:11, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
> Am 08.12.2020 um 09:25 schrieb Kai-Heng Feng:
>> If we use sysfs to disable L1 ASPM, then enable one L1 ASPM substate
>> again, all other substates will also be enabled too:
>> 
>> link# grep . *
>> clkpm:1
>> l0s_aspm:1
>> l1_1_aspm:1
>> l1_1_pcipm:1
>> l1_2_aspm:1
>> l1_2_pcipm:1
>> l1_aspm:1
>> 
>> link# echo 0 > l1_aspm
>> 
>> link# grep . *
>> clkpm:1
>> l0s_aspm:1
>> l1_1_aspm:0
>> l1_1_pcipm:0
>> l1_2_aspm:0
>> l1_2_pcipm:0
>> l1_aspm:0
>> 
>> link# echo 1 > l1_2_aspm
>> 
>> link# grep . *
>> clkpm:1
>> l0s_aspm:1
>> l1_1_aspm:1
>> l1_1_pcipm:1
>> l1_2_aspm:1
>> l1_2_pcipm:1
>> l1_aspm:1
>> 
>> This is because disabled ASPM states weren't saved, so enable any of the
>> substate will also enable others.
>> 
>> So store the disabled ASPM states for consistency.
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/pci/pcie/aspm.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index ac0557a305af..2ea9fddadfad 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -658,6 +658,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>> 	/* Setup initial capable state. Will be updated later */
>> 	link->aspm_capable = link->aspm_support;
>> 
>> +	link->aspm_disable = link->aspm_capable & ~link->aspm_default;
>> +
> 
> This makes sense only in combination with patch 2. However I think patch 1
> should be independent of patch 2. Especially if we consider patch 1 a fix
> that is applied to stable whilst patch 2 is an improvement for next.
> 
>> 	/* Get and check endpoint acceptable latencies */
>> 	list_for_each_entry(child, &linkbus->devices, bus_list) {
>> 		u32 reg32, encoding;
>> @@ -1226,11 +1228,15 @@ static ssize_t aspm_attr_store_common(struct device *dev,
>> 	mutex_lock(&aspm_lock);
>> 
>> 	if (state_enable) {
>> -		link->aspm_disable &= ~state;
>> 		/* need to enable L1 for substates */
>> 		if (state & ASPM_STATE_L1SS)
>> -			link->aspm_disable &= ~ASPM_STATE_L1;
>> +			state |= ASPM_STATE_L1;
>> +
>> +		link->aspm_disable &= ~state;
> 
> I don't see what this part of the patch changes. Can you elaborate on why
> this is needed?

No this is just a cosmetic change. Of course "cosmetic" is really subjective.
I'll drop this part in v2.

> 
>> 	} else {
>> +		if (state == ASPM_STATE_L1)
>> +			state |= ASPM_STATE_L1SS;
>> +
> 
> I think this part should be sufficient to fix the behavior. because what
> I think currently happens:
> 
> 1. original status: policy powersupersave, nothing disabled -> L1 + L1SS active
> 2. disable L1: L1 disabled, pcie_config_aspm_link() disabled L1 and L1SS
>   w/o adding L1SS to link-> aspm_disabled
> 3. enable one L1SS state: aspm_attr_store_common() removes L1 from
>   link->aspm_disabled -> link->aspm_disabled is empty, resulting in
>   L1 + L1SS being active

Yes. This is the case the patch solves.

Kai-Heng

> 
>> 		link->aspm_disable |= state;
>> 	}