Re: [PATCH v2 2/5] PCI/ASPM: Set ASPM_STATE_L1 only when driver enables L1.0

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

On 5/2/23 12:31 PM, Ajay Agarwal wrote:
> Currently the core driver sets ASPM_STATE_L1 as well as

I think you can use the term ASPM driver uniformly.

> ASPM_STATE_L1SS when the caller wants to enable just L1.0.

L1?

> This is incorrect. Fix this by setting the ASPM_STATE_L1 bit
> only when the caller wishes to enable L1.0.
> 
> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> ---

Otherwise, looks fine.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> Changelog since v1:
>  - Break down the L1 and L1ss handling into separate patches
> 
>  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 5765b226102a..4ad0bf5d5838 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1170,8 +1170,7 @@ 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;
>  	if (state & PCIE_LINK_STATE_L1_1)
>  		link->aspm_default |= ASPM_STATE_L1_1;
>  	if (state & PCIE_LINK_STATE_L1_2)

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer