[PATCH 0/3] ASPM: aspm_disable/default/support state handling fixes

[PATCH 0/3] ASPM: aspm_disable/default/support state handling fixes

[Ajay Agarwal]

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

I intend to fix these bugs.

Ajay Agarwal (3):
  PCI/ASPM: Disable ASPM_STATE_L1 only when class driver disables L1
    ASPM
  PCI/ASPM: Set ASPM_STATE_L1 when class driver enables L1ss
  PCI/ASPM: Remove unnecessary ASPM_STATE_L1SS check

 drivers/pci/pcie/aspm.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

-- 
2.40.0.577.gac1e443424-goog

[PATCH 1/3] 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 when the class driver 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 class driver disables L1 ASPM.

Signed-off-by: Ajay Agarwal <ajayagarwal@google.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 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.0.577.gac1e443424-goog

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

[Bjorn Helgaas]

On Tue, Apr 11, 2023 at 04:40:32PM +0530, Ajay Agarwal wrote:
> Currently the aspm driver sets ASPM_STATE_L1 as well as
> ASPM_STATE_L1SS bits when the class driver disables L1.

I would have said just "driver" -- do you mean something different by
using "class driver"?  The callers I see are garden-variety drivers
for individual devices like hci_bcm4377, xillybus_pcie, e1000e, jme,
etc.

> 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.

Right.  It'd be nice to combine __pci_disable_link_state() and
aspm_attr_store_common() so they use the same logic for this, but
that's not really trivial to do.

> Disable ASPM_STATE_L1 only when class driver disables L1 ASPM.

So IIUC, this is a cleanup and should not fix any actual function
bugs, right?  If it *does* fix a bug, we should add a Fixes: tag and a
description of the bug.

> Signed-off-by: Ajay Agarwal <ajayagarwal@google.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 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.0.577.gac1e443424-goog
> 

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

[Ajay Agarwal]

On Mon, May 01, 2023 at 12:21:14PM -0500, Bjorn Helgaas wrote:
> On Tue, Apr 11, 2023 at 04:40:32PM +0530, Ajay Agarwal wrote:
> > Currently the aspm driver sets ASPM_STATE_L1 as well as
> > ASPM_STATE_L1SS bits when the class driver disables L1.
> 
> I would have said just "driver" -- do you mean something different by
> using "class driver"?  The callers I see are garden-variety drivers
> for individual devices like hci_bcm4377, xillybus_pcie, e1000e, jme,
> etc.
No, I do not mean anything different by "class driver". I just wanted
to name the caller drivers of the ASPM APIs as something other than
just "driver". Do you want me to change this to "driver" ?
> 
> > 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.
> 
> Right.  It'd be nice to combine __pci_disable_link_state() and
> aspm_attr_store_common() so they use the same logic for this, but
> that's not really trivial to do.
> 
Ack.
> > Disable ASPM_STATE_L1 only when class driver disables L1 ASPM.
> 
> So IIUC, this is a cleanup and should not fix any actual function
> bugs, right?  If it *does* fix a bug, we should add a Fixes: tag and a
> description of the bug.
> 
Yes, this is just a cleanup.
> > Signed-off-by: Ajay Agarwal <ajayagarwal@google.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 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.0.577.gac1e443424-goog
> > 

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

[Bjorn Helgaas]

On Tue, May 02, 2023 at 06:08:13PM +0530, Ajay Agarwal wrote:
> On Mon, May 01, 2023 at 12:21:14PM -0500, Bjorn Helgaas wrote:
> > On Tue, Apr 11, 2023 at 04:40:32PM +0530, Ajay Agarwal wrote:
> > > Currently the aspm driver sets ASPM_STATE_L1 as well as
> > > ASPM_STATE_L1SS bits when the class driver disables L1.
> > 
> > I would have said just "driver" -- do you mean something different by
> > using "class driver"?  The callers I see are garden-variety drivers
> > for individual devices like hci_bcm4377, xillybus_pcie, e1000e, jme,
> > etc.
>
> No, I do not mean anything different by "class driver". I just wanted
> to name the caller drivers of the ASPM APIs as something other than
> just "driver". Do you want me to change this to "driver" ?

Yes, please, I think "driver" by itself is sufficient.  IIUC, "class
driver" generally refers to a generic or abstract driver that provides
a common interface to a variety of different devices.  This interface
could be used by such a class driver or by the driver for a specific
device, but the type of driver is not relevant to this patch.

Bjorn

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

[Ajay Agarwal]

On Tue, May 02, 2023 at 11:07:22AM -0500, Bjorn Helgaas wrote:
> On Tue, May 02, 2023 at 06:08:13PM +0530, Ajay Agarwal wrote:
> > On Mon, May 01, 2023 at 12:21:14PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Apr 11, 2023 at 04:40:32PM +0530, Ajay Agarwal wrote:
> > > > Currently the aspm driver sets ASPM_STATE_L1 as well as
> > > > ASPM_STATE_L1SS bits when the class driver disables L1.
> > > 
> > > I would have said just "driver" -- do you mean something different by
> > > using "class driver"?  The callers I see are garden-variety drivers
> > > for individual devices like hci_bcm4377, xillybus_pcie, e1000e, jme,
> > > etc.
> >
> > No, I do not mean anything different by "class driver". I just wanted
> > to name the caller drivers of the ASPM APIs as something other than
> > just "driver". Do you want me to change this to "driver" ?
> 
> Yes, please, I think "driver" by itself is sufficient.  IIUC, "class
> driver" generally refers to a generic or abstract driver that provides
> a common interface to a variety of different devices.  This interface
> could be used by such a class driver or by the driver for a specific
> device, but the type of driver is not relevant to this patch.
> 
> Bjorn

Ack. Will do in the next revision.

[PATCH 2/3] PCI/ASPM: Set ASPM_STATE_L1 when class driver enables L1ss

[Ajay Agarwal]

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

Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
---
 drivers/pci/pcie/aspm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 5765b226102a..7c9935f331f1 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1170,16 +1170,16 @@ 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;
+	/* 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.0.577.gac1e443424-goog

Re: [PATCH 2/3] PCI/ASPM: Set ASPM_STATE_L1 when class driver enables L1ss

[Bjorn Helgaas]

[+cc Nirmal, Jonathan, since vmd is the only caller of
pci_enable_link_state()]

On Tue, Apr 11, 2023 at 04:40:33PM +0530, Ajay Agarwal wrote:
> Currently the aspm driver does not set ASPM_STATE_L1 bit in
> aspm_default when the class driver requests L1SS ASPM state.
> This will lead to pcie_config_aspm_link() not enabling the
> requested L1SS state. Set ASPM_STATE_L1 when class driver
> enables L1ss.

Since vmd is currently the only caller of pci_enable_link_state(), and
it supplies PCIE_LINK_STATE_ALL:

  #define PCIE_LINK_STATE_ALL (PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
                               PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
                               PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
                               PCIE_LINK_STATE_L1_2_PCIPM)

I don't think this makes any functional difference at this point,
right?

> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> ---
>  drivers/pci/pcie/aspm.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 5765b226102a..7c9935f331f1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1170,16 +1170,16 @@ 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;
> +	/* 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;

IIUC, this:

  pci_enable_link_state(PCIE_LINK_STATE_L1_1)

currently doesn't actually enable L1.1 because the caller didn't
supply "PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1".

I'm not sure that's a problem -- the driver can easily supply both if
it wants both.

For devices that support only L1,
"pci_enable_link_state(PCIE_LINK_STATE_L1_1)" would implicitly enable
L1 even though L1.1 is not supported, which seems a little bit weird.

>  	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.0.577.gac1e443424-goog
> 

Re: [PATCH 2/3] PCI/ASPM: Set ASPM_STATE_L1 when class driver enables L1ss

[Ajay Agarwal]

On Mon, May 01, 2023 at 12:44:39PM -0500, Bjorn Helgaas wrote:
> [+cc Nirmal, Jonathan, since vmd is the only caller of
> pci_enable_link_state()]
> 
> On Tue, Apr 11, 2023 at 04:40:33PM +0530, Ajay Agarwal wrote:
> > Currently the aspm driver does not set ASPM_STATE_L1 bit in
> > aspm_default when the class driver requests L1SS ASPM state.
> > This will lead to pcie_config_aspm_link() not enabling the
> > requested L1SS state. Set ASPM_STATE_L1 when class driver
> > enables L1ss.
> 
> Since vmd is currently the only caller of pci_enable_link_state(), and
> it supplies PCIE_LINK_STATE_ALL:
> 
>   #define PCIE_LINK_STATE_ALL (PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
>                                PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
>                                PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
>                                PCIE_LINK_STATE_L1_2_PCIPM)
> 
> I don't think this makes any functional difference at this point,
> right?
>
Yes, this does not make any functional difference to the vmd driver.
> > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> > ---
> >  drivers/pci/pcie/aspm.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 5765b226102a..7c9935f331f1 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1170,16 +1170,16 @@ 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;
> > +	/* 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;
> 
> IIUC, this:
> 
>   pci_enable_link_state(PCIE_LINK_STATE_L1_1)
> 
> currently doesn't actually enable L1.1 because the caller didn't
> supply "PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1".
> 
> I'm not sure that's a problem -- the driver can easily supply both if
> it wants both.
> 
Consider this: A driver wants to enable L1.1. So it calls:
    pci_enable_link_state(PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1)
The current logic will end up enabling L1.2 as well. The driver does
not want that.

Also, we should be letting the ASPM core driver handle the logic that
L1.0 needs to be set for L1.1/L1.2 to happen, instead of putting that
responsibility to the caller driver.
> For devices that support only L1,
> "pci_enable_link_state(PCIE_LINK_STATE_L1_1)" would implicitly enable
> L1 even though L1.1 is not supported, which seems a little bit weird.
>
If L1.1 is not supported, then ASPM_STATE_L1_1 will not be set in
`aspm_capable` right? That will not allow L1.1 to be enabled. So, we
should be fine.
> >  	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.0.577.gac1e443424-goog
> > 

Re: [PATCH 2/3] PCI/ASPM: Set ASPM_STATE_L1 when class driver enables L1ss

[Bjorn Helgaas]

On Tue, May 02, 2023 at 06:32:50PM +0530, Ajay Agarwal wrote:
> On Mon, May 01, 2023 at 12:44:39PM -0500, Bjorn Helgaas wrote:
> > On Tue, Apr 11, 2023 at 04:40:33PM +0530, Ajay Agarwal wrote:
> > > Currently the aspm driver does not set ASPM_STATE_L1 bit in
> > > aspm_default when the class driver requests L1SS ASPM state.
> > > This will lead to pcie_config_aspm_link() not enabling the
> > > requested L1SS state. Set ASPM_STATE_L1 when class driver
> > > enables L1ss.
> > 
> > Since vmd is currently the only caller of pci_enable_link_state(), and
> > it supplies PCIE_LINK_STATE_ALL:
> > 
> >   #define PCIE_LINK_STATE_ALL (PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
> >                                PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
> >                                PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
> >                                PCIE_LINK_STATE_L1_2_PCIPM)
> > 
> > I don't think this makes any functional difference at this point,
> > right?
>
> Yes, this does not make any functional difference to the vmd driver.
> ...

> > > @@ -1170,16 +1170,16 @@ 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;
> > > +	/* 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;
> > 
> > IIUC, this:
> > 
> >   pci_enable_link_state(PCIE_LINK_STATE_L1_1)
> > 
> > currently doesn't actually enable L1.1 because the caller didn't
> > supply "PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1".
> > 
> > I'm not sure that's a problem -- the driver can easily supply both if
> > it wants both.
>
> Consider this: A driver wants to enable L1.1. So it calls:
>     pci_enable_link_state(PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1)
> The current logic will end up enabling L1.2 as well. The driver does
> not want that.

Hmmm, I think I see what you mean.  ASPM_STATE_L1SS includes both
ASPM_STATE_L1_1 and ASPM_STATE_L1_2:

  #define ASPM_STATE_L1_2_MASK    (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM)
  #define ASPM_STATE_L1SS         (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\
				   ASPM_STATE_L1_2_MASK)

so this sets ASPM_STATE_L1_1 and ASPM_STATE_L1_2:

  if (state & PCIE_LINK_STATE_L1)
    link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;

which makes it pointless for a caller to supply PCIE_LINK_STATE_L1_1
or PCIE_LINK_STATE_L1_2:

  if (state & PCIE_LINK_STATE_L1_1)
    link->aspm_default |= ASPM_STATE_L1_1;
  if (state & PCIE_LINK_STATE_L1_2)
    link->aspm_default |= ASPM_STATE_L1_2;

> Also, we should be letting the ASPM core driver handle the logic that
> L1.0 needs to be set for L1.1/L1.2 to happen, instead of putting that
> responsibility to the caller driver.
>
> > For devices that support only L1,
> > "pci_enable_link_state(PCIE_LINK_STATE_L1_1)" would implicitly enable
> > L1 even though L1.1 is not supported, which seems a little bit weird.
> >
> If L1.1 is not supported, then ASPM_STATE_L1_1 will not be set in
> `aspm_capable` right? That will not allow L1.1 to be enabled. So, we
> should be fine.

It seems like there are two questions here:

  1) We currently enable L1.2 when the caller didn't request it.  This
  seems clearly wrong and we should fix it.  If we can make a patch
  that does just this part, that would be good.

  2) Should the PCI core enable L1 if the caller requests only L1.1
  (or L1.2)?  This one isn't as clear to me, but there's only one
  caller, and whatever we do won't make a difference to it, so it can
  go either way.  If we want to make a semantic change here, that's
  OK, but I'd like to make that its own patch if possible.

> > >  	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.0.577.gac1e443424-goog
> > > 

Re: [PATCH 2/3] PCI/ASPM: Set ASPM_STATE_L1 when class driver enables L1ss

[Ajay Agarwal]

On Tue, May 02, 2023 at 11:02:24AM -0500, Bjorn Helgaas wrote:
> On Tue, May 02, 2023 at 06:32:50PM +0530, Ajay Agarwal wrote:
> > On Mon, May 01, 2023 at 12:44:39PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Apr 11, 2023 at 04:40:33PM +0530, Ajay Agarwal wrote:
> > > > Currently the aspm driver does not set ASPM_STATE_L1 bit in
> > > > aspm_default when the class driver requests L1SS ASPM state.
> > > > This will lead to pcie_config_aspm_link() not enabling the
> > > > requested L1SS state. Set ASPM_STATE_L1 when class driver
> > > > enables L1ss.
> > > 
> > > Since vmd is currently the only caller of pci_enable_link_state(), and
> > > it supplies PCIE_LINK_STATE_ALL:
> > > 
> > >   #define PCIE_LINK_STATE_ALL (PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
> > >                                PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
> > >                                PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
> > >                                PCIE_LINK_STATE_L1_2_PCIPM)
> > > 
> > > I don't think this makes any functional difference at this point,
> > > right?
> >
> > Yes, this does not make any functional difference to the vmd driver.
> > ...
> 
> > > > @@ -1170,16 +1170,16 @@ 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;
> > > > +	/* 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;
> > > 
> > > IIUC, this:
> > > 
> > >   pci_enable_link_state(PCIE_LINK_STATE_L1_1)
> > > 
> > > currently doesn't actually enable L1.1 because the caller didn't
> > > supply "PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1".
> > > 
> > > I'm not sure that's a problem -- the driver can easily supply both if
> > > it wants both.
> >
> > Consider this: A driver wants to enable L1.1. So it calls:
> >     pci_enable_link_state(PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1)
> > The current logic will end up enabling L1.2 as well. The driver does
> > not want that.
> 
> Hmmm, I think I see what you mean.  ASPM_STATE_L1SS includes both
> ASPM_STATE_L1_1 and ASPM_STATE_L1_2:
> 
>   #define ASPM_STATE_L1_2_MASK    (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM)
>   #define ASPM_STATE_L1SS         (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\
> 				   ASPM_STATE_L1_2_MASK)
> 
> so this sets ASPM_STATE_L1_1 and ASPM_STATE_L1_2:
> 
>   if (state & PCIE_LINK_STATE_L1)
>     link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> 
> which makes it pointless for a caller to supply PCIE_LINK_STATE_L1_1
> or PCIE_LINK_STATE_L1_2:
> 
>   if (state & PCIE_LINK_STATE_L1_1)
>     link->aspm_default |= ASPM_STATE_L1_1;
>   if (state & PCIE_LINK_STATE_L1_2)
>     link->aspm_default |= ASPM_STATE_L1_2;
> 
> > Also, we should be letting the ASPM core driver handle the logic that
> > L1.0 needs to be set for L1.1/L1.2 to happen, instead of putting that
> > responsibility to the caller driver.
> >
> > > For devices that support only L1,
> > > "pci_enable_link_state(PCIE_LINK_STATE_L1_1)" would implicitly enable
> > > L1 even though L1.1 is not supported, which seems a little bit weird.
> > >
> > If L1.1 is not supported, then ASPM_STATE_L1_1 will not be set in
> > `aspm_capable` right? That will not allow L1.1 to be enabled. So, we
> > should be fine.
> 
> It seems like there are two questions here:
> 
>   1) We currently enable L1.2 when the caller didn't request it.  This
>   seems clearly wrong and we should fix it.  If we can make a patch
>   that does just this part, that would be good.
>
Ack. Will do in the next revision.

>   2) Should the PCI core enable L1 if the caller requests only L1.1
>   (or L1.2)?  This one isn't as clear to me, but there's only one
>   caller, and whatever we do won't make a difference to it, so it can
>   go either way.  If we want to make a semantic change here, that's
>   OK, but I'd like to make that its own patch if possible.
>
Ack. Will create a new patch in the next revision.

> > > >  	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.0.577.gac1e443424-goog
> > > > 

[PATCH 3/3] PCI/ASPM: Remove unnecessary ASPM_STATE_L1SS check

[Ajay Agarwal]

Currently the driver checks if ASPM_STATE_L1SS is supported
before calling aspm_calc_l1ss_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>
---
 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 7c9935f331f1..8c45835e8016 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -481,9 +481,6 @@ static void aspm_calc_l1ss_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_l1ss_info(link, parent_l1ss_cap, child_l1ss_cap);
 }
 
-- 
2.40.0.577.gac1e443424-goog

Re: [PATCH 3/3] PCI/ASPM: Remove unnecessary ASPM_STATE_L1SS check

[Bjorn Helgaas]

On Tue, Apr 11, 2023 at 04:40:34PM +0530, Ajay Agarwal wrote:
> Currently the driver checks if ASPM_STATE_L1SS is supported
> before calling aspm_calc_l1ss_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>
> ---
>  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 7c9935f331f1..8c45835e8016 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -481,9 +481,6 @@ static void aspm_calc_l1ss_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_l1ss_info(link, parent_l1ss_cap, child_l1ss_cap);

I think the reason it was this way is because several of the relevant
names use "l1ss":

  ASPM_STATE_L1SS
  aspm_calc_l1ss_info
  calc_l1ss_pwron

But everything in aspm_calc_l1ss_info() is L1.2-specific, so I think
it would make sense to use your patch, and at the same time, rename
aspm_calc_l1ss_info() and calc_l1ss_pwron() to aspm_calc_l12_info()
and calc_l12_pwron() to match.

Bjorn

Re: [PATCH 3/3] PCI/ASPM: Remove unnecessary ASPM_STATE_L1SS check

[Ajay Agarwal]

On Mon, May 01, 2023 at 12:55:18PM -0500, Bjorn Helgaas wrote:
> On Tue, Apr 11, 2023 at 04:40:34PM +0530, Ajay Agarwal wrote:
> > Currently the driver checks if ASPM_STATE_L1SS is supported
> > before calling aspm_calc_l1ss_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>
> > ---
> >  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 7c9935f331f1..8c45835e8016 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -481,9 +481,6 @@ static void aspm_calc_l1ss_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_l1ss_info(link, parent_l1ss_cap, child_l1ss_cap);
> 
> I think the reason it was this way is because several of the relevant
> names use "l1ss":
> 
>   ASPM_STATE_L1SS
>   aspm_calc_l1ss_info
>   calc_l1ss_pwron
> 
> But everything in aspm_calc_l1ss_info() is L1.2-specific, so I think
> it would make sense to use your patch, and at the same time, rename
> aspm_calc_l1ss_info() and calc_l1ss_pwron() to aspm_calc_l12_info()
> and calc_l12_pwron() to match.
> 
> Bjorn
Sure, will incorporate your suggestions in the next version.

[PATCH 0/3] ASPM: aspm_disable/default/support state handling fixes

[Ajay Agarwal]

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

I intend to fix these bugs.

Ajay Agarwal (3):
  PCI/ASPM: Disable ASPM_STATE_L1 only when class driver disables L1
    ASPM
  PCI/ASPM: Set ASPM_STATE_L1 when class driver enables L1ss
  PCI/ASPM: Remove unnecessary ASPM_STATE_L1SS check

 drivers/pci/pcie/aspm.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

-- 
2.40.0.577.gac1e443424-goog

Re: [PATCH 0/3] ASPM: aspm_disable/default/support state handling fixes

[Ajay Agarwal]

Please ignore this email. It was sent by mistake.

On Wed, Apr 12, 2023 at 3:03 PM Ajay Agarwal <ajayagarwal@google.com> wrote:
>
> On going through the aspm driver, I found some potential bugs in
> the way the aspm_disable, aspm_default and aspm_support states
> are being handled by the driver.
>
> I intend to fix these bugs.
>
> Ajay Agarwal (3):
>   PCI/ASPM: Disable ASPM_STATE_L1 only when class driver disables L1
>     ASPM
>   PCI/ASPM: Set ASPM_STATE_L1 when class driver enables L1ss
>   PCI/ASPM: Remove unnecessary ASPM_STATE_L1SS check
>
>  drivers/pci/pcie/aspm.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> --
> 2.40.0.577.gac1e443424-goog
>