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

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

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.

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