[PATCH v2 0/5] ASPM: aspm_disable/default state handling fixes

[PATCH v2 0/5] ASPM: aspm_disable/default state handling fixes

[Ajay Agarwal]

On going through the aspm driver, I found some potential bugs
and opportunities for code cleanup in the way the aspm_disable
and aspm_default states are being handled by the driver.

Changes from v1 to v2:
 - Split the patches into smaller patches
 - Add the patch to rename L1.2 specific functions

Ajay Agarwal (5):
  PCI/ASPM: Disable ASPM_STATE_L1 only when class driver disables L1
    ASPM
  PCI/ASPM: Set ASPM_STATE_L1 only when driver enables L1.0
  PCI/ASPM: Set ASPM_STATE_L1 when driver enables L1ss
  PCI/ASPM: Rename L1.2 specific functions
  PCI/ASPM: Remove unnecessary ASPM_STATE_L1SS check

 drivers/pci/pcie/aspm.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

-- 
2.40.1.495.gc816e09b53d-goog

[PATCH v2 1/5] PCI/ASPM: Disable ASPM_STATE_L1 only when class driver disables L1 ASPM

[Ajay Agarwal]

Currently the aspm driver sets ASPM_STATE_L1 as well as
ASPM_STATE_L1SS bits in aspm_disable when the caller disables L1.
pcie_config_aspm_link takes care that L1ss ASPM is not enabled
if L1 is disabled. ASPM_STATE_L1SS bits do not need to be
explicitly set. The sysfs node store() function, which also
modifies the aspm_disable value, does not set these bits either
when only L1 ASPM is disabled by the user.

Disable ASPM_STATE_L1 only when the caller disables L1 ASPM.

Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
---
Changelog since v1:
 - Better commit message

 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 66d7514ca111..5765b226102a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1095,8 +1095,7 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
 	if (state & PCIE_LINK_STATE_L0S)
 		link->aspm_disable |= ASPM_STATE_L0S;
 	if (state & PCIE_LINK_STATE_L1)
-		/* L1 PM substates require L1 */
-		link->aspm_disable |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
+		link->aspm_disable |= ASPM_STATE_L1;
 	if (state & PCIE_LINK_STATE_L1_1)
 		link->aspm_disable |= ASPM_STATE_L1_1;
 	if (state & PCIE_LINK_STATE_L1_2)
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH v2 2/5] PCI/ASPM: Set ASPM_STATE_L1 only when driver enables L1.0

[Ajay Agarwal]

Currently the core driver sets ASPM_STATE_L1 as well as
ASPM_STATE_L1SS when the caller wants to enable just L1.0.
This is incorrect. Fix this by setting the ASPM_STATE_L1 bit
only when the caller wishes to enable L1.0.

Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
---
Changelog since v1:
 - Break down the L1 and L1ss handling into separate patches

 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 5765b226102a..4ad0bf5d5838 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1170,8 +1170,7 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
 	if (state & PCIE_LINK_STATE_L0S)
 		link->aspm_default |= ASPM_STATE_L0S;
 	if (state & PCIE_LINK_STATE_L1)
-		/* L1 PM substates require L1 */
-		link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
+		link->aspm_default |= ASPM_STATE_L1;
 	if (state & PCIE_LINK_STATE_L1_1)
 		link->aspm_default |= ASPM_STATE_L1_1;
 	if (state & PCIE_LINK_STATE_L1_2)
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH v2 3/5] PCI/ASPM: Set ASPM_STATE_L1 when driver enables L1ss

[Ajay Agarwal]

Currently the aspm driver does not set ASPM_STATE_L1 bit in
aspm_default when the caller requests L1SS ASPM state. This will
lead to pcie_config_aspm_link() not enabling the requested L1SS
state. Set ASPM_STATE_L1 when driver enables L1ss.

Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
---
Changelog since v1:
 - Break down the L1 and L1ss handling into separate patches

 drivers/pci/pcie/aspm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 4ad0bf5d5838..7c9935f331f1 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1171,14 +1171,15 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
 		link->aspm_default |= ASPM_STATE_L0S;
 	if (state & PCIE_LINK_STATE_L1)
 		link->aspm_default |= ASPM_STATE_L1;
+	/* L1 PM substates require L1 */
 	if (state & PCIE_LINK_STATE_L1_1)
-		link->aspm_default |= ASPM_STATE_L1_1;
+		link->aspm_default |= ASPM_STATE_L1_1 | ASPM_STATE_L1;
 	if (state & PCIE_LINK_STATE_L1_2)
-		link->aspm_default |= ASPM_STATE_L1_2;
+		link->aspm_default |= ASPM_STATE_L1_2 | ASPM_STATE_L1;
 	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
-		link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
+		link->aspm_default |= ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1;
 	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
-		link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
+		link->aspm_default |= ASPM_STATE_L1_2_PCIPM | ASPM_STATE_L1;
 	pcie_config_aspm_link(link, policy_to_aspm_state(link));
 
 	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH v2 4/5] PCI/ASPM: Rename L1.2 specific functions

[Ajay Agarwal]

The functions aspm_calc_l1ss_info() and calc_l1ss_pwron() perform
calculations and register programming specific to L1.2 state.
Rename them to aspm_calc_l12_info() and calc_l12_pwron()
respectively.

Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
---
Changelog since v1:
 - New patch to rename L1.2 specific functions

 drivers/pci/pcie/aspm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 7c9935f331f1..db7c369a0544 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -337,7 +337,7 @@ static u32 calc_l1_acceptable(u32 encoding)
 }
 
 /* Convert L1SS T_pwr encoding to usec */
-static u32 calc_l1ss_pwron(struct pci_dev *pdev, u32 scale, u32 val)
+static u32 calc_l12_pwron(struct pci_dev *pdev, u32 scale, u32 val)
 {
 	switch (scale) {
 	case 0:
@@ -471,7 +471,7 @@ static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
 }
 
 /* Calculate L1.2 PM substate timing parameters */
-static void aspm_calc_l1ss_info(struct pcie_link_state *link,
+static void aspm_calc_l12_info(struct pcie_link_state *link,
 				u32 parent_l1ss_cap, u32 child_l1ss_cap)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
@@ -495,13 +495,13 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 	val2   = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
 	scale2 = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
 
-	if (calc_l1ss_pwron(parent, scale1, val1) >
-	    calc_l1ss_pwron(child, scale2, val2)) {
+	if (calc_l12_pwron(parent, scale1, val1) >
+	    calc_l12_pwron(child, scale2, val2)) {
 		ctl2 |= scale1 | (val1 << 3);
-		t_power_on = calc_l1ss_pwron(parent, scale1, val1);
+		t_power_on = calc_l12_pwron(parent, scale1, val1);
 	} else {
 		ctl2 |= scale2 | (val2 << 3);
-		t_power_on = calc_l1ss_pwron(child, scale2, val2);
+		t_power_on = calc_l12_pwron(child, scale2, val2);
 	}
 
 	/*
@@ -617,7 +617,7 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
 		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
 
 	if (link->aspm_support & ASPM_STATE_L1SS)
-		aspm_calc_l1ss_info(link, parent_l1ss_cap, child_l1ss_cap);
+		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
 }
 
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
-- 
2.40.1.495.gc816e09b53d-goog

[PATCH v2 5/5] PCI/ASPM: Remove unnecessary ASPM_STATE_L1SS check

[Ajay Agarwal]

Currently the driver checks if ASPM_STATE_L1SS is supported
before calling aspm_calc_l12_info(), only for this function to
return if ASPM_STATE_L1_2_MASK is not supported. Simplify the
logic by directly checking for L1.2 mask.

Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
---
Changelog since v1:
 - Rebase on top of the L1.2 function rename patch

 drivers/pci/pcie/aspm.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index db7c369a0544..e89091cba356 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -481,9 +481,6 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 	u32 pctl1, pctl2, cctl1, cctl2;
 	u32 pl1_2_enables, cl1_2_enables;
 
-	if (!(link->aspm_support & ASPM_STATE_L1_2_MASK))
-		return;
-
 	/* Choose the greater of the two Port Common_Mode_Restore_Times */
 	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
 	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
@@ -616,7 +613,7 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
 	if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2)
 		link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM;
 
-	if (link->aspm_support & ASPM_STATE_L1SS)
+	if (link->aspm_support & ASPM_STATE_L1_2_MASK)
 		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
 }
 
-- 
2.40.1.495.gc816e09b53d-goog

Re: [PATCH v2 1/5] PCI/ASPM: Disable ASPM_STATE_L1 only when class driver disables L1 ASPM

[Sathyanarayanan Kuppuswamy]

On 5/2/23 12:31 PM, Ajay Agarwal wrote:
> Currently the aspm driver sets ASPM_STATE_L1 as well as
> ASPM_STATE_L1SS bits in aspm_disable when the caller disables L1.
> pcie_config_aspm_link takes care that L1ss ASPM is not enabled
> if L1 is disabled. ASPM_STATE_L1SS bits do not need to be
> explicitly set. The sysfs node store() function, which also
> modifies the aspm_disable value, does not set these bits either
> when only L1 ASPM is disabled by the user.
> 
> Disable ASPM_STATE_L1 only when the caller disables L1 ASPM.

Maybe you can add something like, No functional changes intended.

Otherwise, looks good.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> 
> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> ---
> Changelog since v1:
>  - Better commit message
> 
>  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 66d7514ca111..5765b226102a 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1095,8 +1095,7 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
>  	if (state & PCIE_LINK_STATE_L0S)
>  		link->aspm_disable |= ASPM_STATE_L0S;
>  	if (state & PCIE_LINK_STATE_L1)
> -		/* L1 PM substates require L1 */
> -		link->aspm_disable |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> +		link->aspm_disable |= ASPM_STATE_L1;
>  	if (state & PCIE_LINK_STATE_L1_1)
>  		link->aspm_disable |= ASPM_STATE_L1_1;
>  	if (state & PCIE_LINK_STATE_L1_2)

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 2/5] PCI/ASPM: Set ASPM_STATE_L1 only when driver enables L1.0

[Sathyanarayanan Kuppuswamy]

On 5/2/23 12:31 PM, Ajay Agarwal wrote:
> Currently the core driver sets ASPM_STATE_L1 as well as

I think you can use the term ASPM driver uniformly.

> ASPM_STATE_L1SS when the caller wants to enable just L1.0.

L1?

> This is incorrect. Fix this by setting the ASPM_STATE_L1 bit
> only when the caller wishes to enable L1.0.
> 
> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> ---

Otherwise, looks fine.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> Changelog since v1:
>  - Break down the L1 and L1ss handling into separate patches
> 
>  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 5765b226102a..4ad0bf5d5838 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1170,8 +1170,7 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
>  	if (state & PCIE_LINK_STATE_L0S)
>  		link->aspm_default |= ASPM_STATE_L0S;
>  	if (state & PCIE_LINK_STATE_L1)
> -		/* L1 PM substates require L1 */
> -		link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> +		link->aspm_default |= ASPM_STATE_L1;
>  	if (state & PCIE_LINK_STATE_L1_1)
>  		link->aspm_default |= ASPM_STATE_L1_1;
>  	if (state & PCIE_LINK_STATE_L1_2)

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 3/5] PCI/ASPM: Set ASPM_STATE_L1 when driver enables L1ss

[Sathyanarayanan Kuppuswamy]

On 5/2/23 12:31 PM, Ajay Agarwal wrote:
> Currently the aspm driver does not set ASPM_STATE_L1 bit in
> aspm_default when the caller requests L1SS ASPM state. This will
> lead to pcie_config_aspm_link() not enabling the requested L1SS
> state. Set ASPM_STATE_L1 when driver enables L1ss.
> 

Is there a bug associated with this issue?

> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> ---
> Changelog since v1:
>  - Break down the L1 and L1ss handling into separate patches
> 
>  drivers/pci/pcie/aspm.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 4ad0bf5d5838..7c9935f331f1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1171,14 +1171,15 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
>  		link->aspm_default |= ASPM_STATE_L0S;
>  	if (state & PCIE_LINK_STATE_L1)
>  		link->aspm_default |= ASPM_STATE_L1;
> +	/* L1 PM substates require L1 */
>  	if (state & PCIE_LINK_STATE_L1_1)
> -		link->aspm_default |= ASPM_STATE_L1_1;
> +		link->aspm_default |= ASPM_STATE_L1_1 | ASPM_STATE_L1;
>  	if (state & PCIE_LINK_STATE_L1_2)
> -		link->aspm_default |= ASPM_STATE_L1_2;
> +		link->aspm_default |= ASPM_STATE_L1_2 | ASPM_STATE_L1;
>  	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
> -		link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
> +		link->aspm_default |= ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1;
>  	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
> -		link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
> +		link->aspm_default |= ASPM_STATE_L1_2_PCIPM | ASPM_STATE_L1;
>  	pcie_config_aspm_link(link, policy_to_aspm_state(link));
>  
>  	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 1/5] PCI/ASPM: Disable ASPM_STATE_L1 only when class driver disables L1 ASPM

[Ajay Agarwal]

On Tue, May 02, 2023 at 06:10:21PM -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> 
> On 5/2/23 12:31 PM, Ajay Agarwal wrote:
> > Currently the aspm driver sets ASPM_STATE_L1 as well as
> > ASPM_STATE_L1SS bits in aspm_disable when the caller disables L1.
> > pcie_config_aspm_link takes care that L1ss ASPM is not enabled
> > if L1 is disabled. ASPM_STATE_L1SS bits do not need to be
> > explicitly set. The sysfs node store() function, which also
> > modifies the aspm_disable value, does not set these bits either
> > when only L1 ASPM is disabled by the user.
> > 
> > Disable ASPM_STATE_L1 only when the caller disables L1 ASPM.
> 
> Maybe you can add something like, No functional changes intended.
> 
Ack. Will do in the next version.

> Otherwise, looks good.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> > 
> > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> > ---
> > Changelog since v1:
> >  - Better commit message
> > 
> >  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 66d7514ca111..5765b226102a 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1095,8 +1095,7 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
> >  	if (state & PCIE_LINK_STATE_L0S)
> >  		link->aspm_disable |= ASPM_STATE_L0S;
> >  	if (state & PCIE_LINK_STATE_L1)
> > -		/* L1 PM substates require L1 */
> > -		link->aspm_disable |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> > +		link->aspm_disable |= ASPM_STATE_L1;
> >  	if (state & PCIE_LINK_STATE_L1_1)
> >  		link->aspm_disable |= ASPM_STATE_L1_1;
> >  	if (state & PCIE_LINK_STATE_L1_2)
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer

Re: [PATCH v2 2/5] PCI/ASPM: Set ASPM_STATE_L1 only when driver enables L1.0

[Ajay Agarwal]

On Tue, May 02, 2023 at 06:17:31PM -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> 
> On 5/2/23 12:31 PM, Ajay Agarwal wrote:
> > Currently the core driver sets ASPM_STATE_L1 as well as
> 
> I think you can use the term ASPM driver uniformly.
>
Ack. Will change in the next version.

> > ASPM_STATE_L1SS when the caller wants to enable just L1.0.
> 
> L1?
> 
Ack. Will change in the next version.

> > This is incorrect. Fix this by setting the ASPM_STATE_L1 bit
> > only when the caller wishes to enable L1.0.
> > 
> > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> > ---
> 
> Otherwise, looks fine.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> > Changelog since v1:
> >  - Break down the L1 and L1ss handling into separate patches
> > 
> >  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 5765b226102a..4ad0bf5d5838 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1170,8 +1170,7 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
> >  	if (state & PCIE_LINK_STATE_L0S)
> >  		link->aspm_default |= ASPM_STATE_L0S;
> >  	if (state & PCIE_LINK_STATE_L1)
> > -		/* L1 PM substates require L1 */
> > -		link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> > +		link->aspm_default |= ASPM_STATE_L1;
> >  	if (state & PCIE_LINK_STATE_L1_1)
> >  		link->aspm_default |= ASPM_STATE_L1_1;
> >  	if (state & PCIE_LINK_STATE_L1_2)
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer

Re: [PATCH v2 3/5] PCI/ASPM: Set ASPM_STATE_L1 when driver enables L1ss

[Ajay Agarwal]

On Tue, May 02, 2023 at 06:18:56PM -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> 
> On 5/2/23 12:31 PM, Ajay Agarwal wrote:
> > Currently the aspm driver does not set ASPM_STATE_L1 bit in
> > aspm_default when the caller requests L1SS ASPM state. This will
> > lead to pcie_config_aspm_link() not enabling the requested L1SS
> > state. Set ASPM_STATE_L1 when driver enables L1ss.
> > 
> 
> Is there a bug associated with this issue?
> 
There is no bug associated. I found this through my dry run of the code.

> > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> > ---
> > Changelog since v1:
> >  - Break down the L1 and L1ss handling into separate patches
> > 
> >  drivers/pci/pcie/aspm.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 4ad0bf5d5838..7c9935f331f1 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1171,14 +1171,15 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
> >  		link->aspm_default |= ASPM_STATE_L0S;
> >  	if (state & PCIE_LINK_STATE_L1)
> >  		link->aspm_default |= ASPM_STATE_L1;
> > +	/* L1 PM substates require L1 */
> >  	if (state & PCIE_LINK_STATE_L1_1)
> > -		link->aspm_default |= ASPM_STATE_L1_1;
> > +		link->aspm_default |= ASPM_STATE_L1_1 | ASPM_STATE_L1;
> >  	if (state & PCIE_LINK_STATE_L1_2)
> > -		link->aspm_default |= ASPM_STATE_L1_2;
> > +		link->aspm_default |= ASPM_STATE_L1_2 | ASPM_STATE_L1;
> >  	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
> > -		link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
> > +		link->aspm_default |= ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1;
> >  	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
> > -		link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
> > +		link->aspm_default |= ASPM_STATE_L1_2_PCIPM | ASPM_STATE_L1;
> >  	pcie_config_aspm_link(link, policy_to_aspm_state(link));
> >  
> >  	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer