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

[Ajay Agarwal <ajayagarwal@google.com>]

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