| Message ID | 20230918131103.24119-5-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | PCI/ASPM: Make ASPM in core robust and remove driver workarounds | expand |
On Mon, Sep 18, 2023 at 04:10:54PM +0300, Ilpo Järvinen wrote: > ASPM service driver does the same L0S / L1S / sub states allowed > calculation in __pci_disable_link_state() and > pci_set_default_link_state(). Is there a typo or something here? This patch only adds a call to __pci_disable_link_state(), not to pci_set_default_link_state(). > Create a helper to calculate the mask for the allowed states. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > drivers/pci/pcie/aspm.c | 33 +++++++++++++++++++++------------ > 1 file changed, 21 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index ec6d7a092ac1..91dc95aca90f 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -1034,6 +1034,26 @@ static struct pcie_link_state *pcie_aspm_get_link(struct pci_dev *pdev) > return bridge->link_state; > } > > +static u8 pci_link_state_mask(int state) > +{ > + u8 result = 0; > + > + if (state & PCIE_LINK_STATE_L0S) > + result |= ASPM_STATE_L0S; > + if (state & PCIE_LINK_STATE_L1) > + result |= ASPM_STATE_L1; > + if (state & PCIE_LINK_STATE_L1_1) > + result |= ASPM_STATE_L1_1; > + if (state & PCIE_LINK_STATE_L1_2) > + result |= ASPM_STATE_L1_2; > + if (state & PCIE_LINK_STATE_L1_1_PCIPM) > + result |= ASPM_STATE_L1_1_PCIPM; > + if (state & PCIE_LINK_STATE_L1_2_PCIPM) > + result |= ASPM_STATE_L1_2_PCIPM; > + > + return result; > +} > + > static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) > { > struct pcie_link_state *link = pcie_aspm_get_link(pdev); > @@ -1063,18 +1083,7 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) > if (sem) > down_read(&pci_bus_sem); > mutex_lock(&aspm_lock); > - if (state & PCIE_LINK_STATE_L0S) > - link->aspm_disable |= ASPM_STATE_L0S; > - if (state & PCIE_LINK_STATE_L1) > - 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) > - link->aspm_disable |= ASPM_STATE_L1_2; > - if (state & PCIE_LINK_STATE_L1_1_PCIPM) > - link->aspm_disable |= ASPM_STATE_L1_1_PCIPM; > - if (state & PCIE_LINK_STATE_L1_2_PCIPM) > - link->aspm_disable |= ASPM_STATE_L1_2_PCIPM; > + link->aspm_disable |= pci_link_state_mask(state); > pcie_config_aspm_link(link, policy_to_aspm_state(link)); > > if (state & PCIE_LINK_STATE_CLKPM) > -- > 2.30.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > On Mon, Sep 18, 2023 at 04:10:54PM +0300, Ilpo Järvinen wrote: > > ASPM service driver does the same L0S / L1S / sub states allowed > > calculation in __pci_disable_link_state() and > > pci_set_default_link_state(). > > Is there a typo or something here? This patch only adds a call to > __pci_disable_link_state(), not to pci_set_default_link_state(). This was because one of the changes that got included in the meantime made the state handling in those two functions to differ so I removed the call from the code but forgot to update the changelog to match the code. I'll fix the changelog.
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index ec6d7a092ac1..91dc95aca90f 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -1034,6 +1034,26 @@ static struct pcie_link_state *pcie_aspm_get_link(struct pci_dev *pdev) return bridge->link_state; } +static u8 pci_link_state_mask(int state) +{ + u8 result = 0; + + if (state & PCIE_LINK_STATE_L0S) + result |= ASPM_STATE_L0S; + if (state & PCIE_LINK_STATE_L1) + result |= ASPM_STATE_L1; + if (state & PCIE_LINK_STATE_L1_1) + result |= ASPM_STATE_L1_1; + if (state & PCIE_LINK_STATE_L1_2) + result |= ASPM_STATE_L1_2; + if (state & PCIE_LINK_STATE_L1_1_PCIPM) + result |= ASPM_STATE_L1_1_PCIPM; + if (state & PCIE_LINK_STATE_L1_2_PCIPM) + result |= ASPM_STATE_L1_2_PCIPM; + + return result; +} + static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) { struct pcie_link_state *link = pcie_aspm_get_link(pdev); @@ -1063,18 +1083,7 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) if (sem) down_read(&pci_bus_sem); mutex_lock(&aspm_lock); - if (state & PCIE_LINK_STATE_L0S) - link->aspm_disable |= ASPM_STATE_L0S; - if (state & PCIE_LINK_STATE_L1) - 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) - link->aspm_disable |= ASPM_STATE_L1_2; - if (state & PCIE_LINK_STATE_L1_1_PCIPM) - link->aspm_disable |= ASPM_STATE_L1_1_PCIPM; - if (state & PCIE_LINK_STATE_L1_2_PCIPM) - link->aspm_disable |= ASPM_STATE_L1_2_PCIPM; + link->aspm_disable |= pci_link_state_mask(state); pcie_config_aspm_link(link, policy_to_aspm_state(link)); if (state & PCIE_LINK_STATE_CLKPM)
ASPM service driver does the same L0S / L1S / sub states allowed calculation in __pci_disable_link_state() and pci_set_default_link_state(). Create a helper to calculate the mask for the allowed states. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- drivers/pci/pcie/aspm.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)