| Message ID | 20230502193140.1062470-4-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 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? > 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;
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
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;
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. 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(-)