| Message ID | 20230502193140.1062470-2-ajayagarwal@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | ASPM: aspm_disable/default state handling fixes | expand |
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)
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
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)
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. 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(-)