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

[Ajay Agarwal <ajayagarwal@google.com>]

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