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

[Ajay Agarwal <ajayagarwal@google.com>]

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