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

[Ajay Agarwal <ajayagarwal@google.com>]

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