| Message ID | 20230918131103.24119-6-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | PCI/ASPM: Make ASPM in core robust and remove driver workarounds | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | fail | "Bluetooth: " prefix is not specified in the subject |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
On Mon, Sep 18, 2023 at 04:10:55PM +0300, Ilpo Järvinen wrote: > pci_disable_link_state() lacks a symmetric pair. Some drivers want to > disable ASPM during certain phases of their operation but then > re-enable it later on. If pci_disable_link_state() is made for the > device, there is currently no way to re-enable the states that were > disabled. pci_disable_link_state() gives drivers a way to disable specified ASPM states using a bitmask (PCIE_LINK_STATE_L0S, PCIE_LINK_STATE_L1, PCIE_LINK_STATE_L1_1, etc), but IIUC the driver can't tell exactly what changed and can't directly restore the original state, e.g., - PCIE_LINK_STATE_L1 enabled initially - driver calls pci_disable_link_state(PCIE_LINK_STATE_L0S) - driver calls pci_enable_link_state(PCIE_LINK_STATE_L0S) - PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 are enabled now Now PCIE_LINK_STATE_L0S is enabled even though it was not initially enabled. Maybe that's what we want; I dunno. pci_disable_link_state() currently returns success/failure, but only r8169 and mt76 even check, and only rtl_init_one() (r8169) has a non-trivial reason, so it's conceivable that it could return a bitmask instead. > Add pci_enable_link_state() to remove ASPM states from the state > disable mask. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 2 ++ > 2 files changed, 44 insertions(+) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 91dc95aca90f..f45d18d47c20 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > } > EXPORT_SYMBOL(pci_disable_link_state); > > +/** > + * pci_enable_link_state - Re-enable device's link state > + * @pdev: PCI device > + * @state: ASPM link states to re-enable > + * > + * Enable device's link state that were previously disable so the link is "state[s] that were previously disable[d]" alludes to the use case you have in mind, but I don't think it describes how this function actually works. This function just makes it possible to enable the specified states. The @state parameter may have nothing to do with any previously disabled states. > + * allowed to enter the specific states. Note that if the BIOS didn't grant > + * ASPM control to the OS, this does nothing because we can't touch the > + * LNKCTL register. > + * > + * Return: 0 or a negative errno. > + */ > +int pci_enable_link_state(struct pci_dev *pdev, int state) > +{ > + struct pcie_link_state *link = pcie_aspm_get_link(pdev); > + > + if (!link) > + return -EINVAL; > + /* > + * A driver requested that ASPM be enabled on this device, but > + * if we don't have permission to manage ASPM (e.g., on ACPI > + * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and > + * the _OSC method), we can't honor that request. > + */ > + if (aspm_disabled) { > + pci_warn(pdev, "can't enable ASPM; OS doesn't have ASPM control\n"); > + return -EPERM; > + } > + > + mutex_lock(&aspm_lock); > + link->aspm_disable &= ~pci_link_state_mask(state); > + pcie_config_aspm_link(link, policy_to_aspm_state(link)); > + > + if (state & PCIE_LINK_STATE_CLKPM) > + link->clkpm_disable = 0; > + pcie_set_clkpm(link, policy_to_clkpm_state(link)); > + mutex_unlock(&aspm_lock); > + > + return 0; > +} > +EXPORT_SYMBOL(pci_enable_link_state); > + > /** > * pci_set_default_link_state - Set the default device link state > * @pdev: PCI device > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 3c24ca164104..844d09230264 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1776,11 +1776,13 @@ extern bool pcie_ports_native; > int pci_disable_link_state(struct pci_dev *pdev, int state); > int pci_disable_link_state_locked(struct pci_dev *pdev, int state); > #ifdef CONFIG_PCIEASPM > +int pci_enable_link_state(struct pci_dev *pdev, int state); > int pci_set_default_link_state(struct pci_dev *pdev, int state); > void pcie_no_aspm(void); > bool pcie_aspm_support_enabled(void); > bool pcie_aspm_enabled(struct pci_dev *pdev); > #else > +static inline int pci_enable_link_state(struct pci_dev *pdev, int state) { return -EOPNOTSUPP; } > static inline int pci_set_default_link_state(struct pci_dev *pdev, int state) > { return 0; } > static inline void pcie_no_aspm(void) { } > -- > 2.30.2 > > > -- > ath12k mailing list > ath12k@lists.infradead.org > https://lists.infradead.org/mailman/listinfo/ath12k
On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > On Mon, Sep 18, 2023 at 04:10:55PM +0300, Ilpo Järvinen wrote: > > pci_disable_link_state() lacks a symmetric pair. Some drivers want to > > disable ASPM during certain phases of their operation but then > > re-enable it later on. If pci_disable_link_state() is made for the > > device, there is currently no way to re-enable the states that were > > disabled. > > pci_disable_link_state() gives drivers a way to disable specified ASPM > states using a bitmask (PCIE_LINK_STATE_L0S, PCIE_LINK_STATE_L1, > PCIE_LINK_STATE_L1_1, etc), but IIUC the driver can't tell exactly > what changed and can't directly restore the original state, e.g., > > - PCIE_LINK_STATE_L1 enabled initially > - driver calls pci_disable_link_state(PCIE_LINK_STATE_L0S) > - driver calls pci_enable_link_state(PCIE_LINK_STATE_L0S) > - PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 are enabled now > > Now PCIE_LINK_STATE_L0S is enabled even though it was not initially > enabled. Maybe that's what we want; I dunno. > > pci_disable_link_state() currently returns success/failure, but only > r8169 and mt76 even check, and only rtl_init_one() (r8169) has a > non-trivial reason, so it's conceivable that it could return a bitmask > instead. It's great that you suggested this since it's actually what also I've been started to think should be done instead of this straightforward approach I used in V2. That is, don't have the drivers to get anything directly from LNKCTL but they should get everything through the API provided by the disable/enable calls which makes it easy for the driver to pass the same value back into the enable call. > > Add pci_enable_link_state() to remove ASPM states from the state > > disable mask. > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > --- > > drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ > > include/linux/pci.h | 2 ++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 91dc95aca90f..f45d18d47c20 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > > } > > EXPORT_SYMBOL(pci_disable_link_state); > > > > +/** > > + * pci_enable_link_state - Re-enable device's link state > > + * @pdev: PCI device > > + * @state: ASPM link states to re-enable > > + * > > + * Enable device's link state that were previously disable so the link is > > "state[s] that were previously disable[d]" alludes to the use case you > have in mind, but I don't think it describes how this function > actually works. This function just makes it possible to enable the > specified states. The @state parameter may have nothing to do with > any previously disabled states. Yes, it's what I've been thinking between the lines. But I see your point that this API didn't make it easy/obvious as is. Would you want me to enforce it too besides altering the API such that the states are actually returned from disable call? (I don't personally find that necessary as long as the API pair itself makes it obvious what the driver is expect to pass there.)
On Thu, Oct 12, 2023 at 03:53:39PM +0300, Ilpo Järvinen wrote: > On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > > On Mon, Sep 18, 2023 at 04:10:55PM +0300, Ilpo Järvinen wrote: > > > pci_disable_link_state() lacks a symmetric pair. Some drivers want to > > > disable ASPM during certain phases of their operation but then > > > re-enable it later on. If pci_disable_link_state() is made for the > > > device, there is currently no way to re-enable the states that were > > > disabled. > > > > pci_disable_link_state() gives drivers a way to disable specified ASPM > > states using a bitmask (PCIE_LINK_STATE_L0S, PCIE_LINK_STATE_L1, > > PCIE_LINK_STATE_L1_1, etc), but IIUC the driver can't tell exactly > > what changed and can't directly restore the original state, e.g., > > > > - PCIE_LINK_STATE_L1 enabled initially > > - driver calls pci_disable_link_state(PCIE_LINK_STATE_L0S) > > - driver calls pci_enable_link_state(PCIE_LINK_STATE_L0S) > > - PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 are enabled now > > > > Now PCIE_LINK_STATE_L0S is enabled even though it was not initially > > enabled. Maybe that's what we want; I dunno. > > > > pci_disable_link_state() currently returns success/failure, but only > > r8169 and mt76 even check, and only rtl_init_one() (r8169) has a > > non-trivial reason, so it's conceivable that it could return a bitmask > > instead. > > It's great that you suggested this since it's actually what also I've been > started to think should be done instead of this straightforward approach > I used in V2. > > That is, don't have the drivers to get anything directly from LNKCTL > but they should get everything through the API provided by the > disable/enable calls which makes it easy for the driver to pass the same > value back into the enable call. > > > > Add pci_enable_link_state() to remove ASPM states from the state > > > disable mask. > > > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > --- > > > drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ > > > include/linux/pci.h | 2 ++ > > > 2 files changed, 44 insertions(+) > > > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > > index 91dc95aca90f..f45d18d47c20 100644 > > > --- a/drivers/pci/pcie/aspm.c > > > +++ b/drivers/pci/pcie/aspm.c > > > @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > > > } > > > EXPORT_SYMBOL(pci_disable_link_state); > > > > > > +/** > > > + * pci_enable_link_state - Re-enable device's link state > > > + * @pdev: PCI device > > > + * @state: ASPM link states to re-enable > > > + * > > > + * Enable device's link state that were previously disable so the link is > > > > "state[s] that were previously disable[d]" alludes to the use case you > > have in mind, but I don't think it describes how this function > > actually works. This function just makes it possible to enable the > > specified states. The @state parameter may have nothing to do with > > any previously disabled states. > > Yes, it's what I've been thinking between the lines. But I see your point > that this API didn't make it easy/obvious as is. > > Would you want me to enforce it too besides altering the API such that the > states are actually returned from disable call? (I don't personally find > that necessary as long as the API pair itself makes it obvious what the > driver is expect to pass there.) This was just a comment about the doc not matching the function behavior. I think we have to support pci_enable_link_state() even if the driver hasn't previously called pci_disable_link_state(), so drivers have to be able to specify the pci_enable_link_state() @state from scratch. Does that answer the enforcement question? I don't think we can really enforce anything other than that @state specifies valid ASPM states. Bjorn
On Fri, 13 Oct 2023, Bjorn Helgaas wrote: > On Thu, Oct 12, 2023 at 03:53:39PM +0300, Ilpo Järvinen wrote: > > On Wed, 11 Oct 2023, Bjorn Helgaas wrote: > > > On Mon, Sep 18, 2023 at 04:10:55PM +0300, Ilpo Järvinen wrote: > > > > pci_disable_link_state() lacks a symmetric pair. Some drivers want to > > > > disable ASPM during certain phases of their operation but then > > > > re-enable it later on. If pci_disable_link_state() is made for the > > > > device, there is currently no way to re-enable the states that were > > > > disabled. > > > > > > pci_disable_link_state() gives drivers a way to disable specified ASPM > > > states using a bitmask (PCIE_LINK_STATE_L0S, PCIE_LINK_STATE_L1, > > > PCIE_LINK_STATE_L1_1, etc), but IIUC the driver can't tell exactly > > > what changed and can't directly restore the original state, e.g., > > > > > > - PCIE_LINK_STATE_L1 enabled initially > > > - driver calls pci_disable_link_state(PCIE_LINK_STATE_L0S) > > > - driver calls pci_enable_link_state(PCIE_LINK_STATE_L0S) > > > - PCIE_LINK_STATE_L0S and PCIE_LINK_STATE_L1 are enabled now > > > > > > Now PCIE_LINK_STATE_L0S is enabled even though it was not initially > > > enabled. Maybe that's what we want; I dunno. > > > > > > pci_disable_link_state() currently returns success/failure, but only > > > r8169 and mt76 even check, and only rtl_init_one() (r8169) has a > > > non-trivial reason, so it's conceivable that it could return a bitmask > > > instead. > > > > It's great that you suggested this since it's actually what also I've been > > started to think should be done instead of this straightforward approach > > I used in V2. > > > > That is, don't have the drivers to get anything directly from LNKCTL > > but they should get everything through the API provided by the > > disable/enable calls which makes it easy for the driver to pass the same > > value back into the enable call. > > > > > > Add pci_enable_link_state() to remove ASPM states from the state > > > > disable mask. > > > > > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > --- > > > > drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/pci.h | 2 ++ > > > > 2 files changed, 44 insertions(+) > > > > > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > > > index 91dc95aca90f..f45d18d47c20 100644 > > > > --- a/drivers/pci/pcie/aspm.c > > > > +++ b/drivers/pci/pcie/aspm.c > > > > @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > > > > } > > > > EXPORT_SYMBOL(pci_disable_link_state); > > > > > > > > +/** > > > > + * pci_enable_link_state - Re-enable device's link state > > > > + * @pdev: PCI device > > > > + * @state: ASPM link states to re-enable > > > > + * > > > > + * Enable device's link state that were previously disable so the link is > > > > > > "state[s] that were previously disable[d]" alludes to the use case you > > > have in mind, but I don't think it describes how this function > > > actually works. This function just makes it possible to enable the > > > specified states. The @state parameter may have nothing to do with > > > any previously disabled states. > > > > Yes, it's what I've been thinking between the lines. But I see your point > > that this API didn't make it easy/obvious as is. > > > > Would you want me to enforce it too besides altering the API such that the > > states are actually returned from disable call? (I don't personally find > > that necessary as long as the API pair itself makes it obvious what the > > driver is expect to pass there.) > > This was just a comment about the doc not matching the function > behavior. > > I think we have to support pci_enable_link_state() even if the driver > hasn't previously called pci_disable_link_state(), so drivers have to > be able to specify the pci_enable_link_state() @state from scratch. > > Does that answer the enforcement question? Yes.
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 91dc95aca90f..f45d18d47c20 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -1117,6 +1117,48 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) } EXPORT_SYMBOL(pci_disable_link_state); +/** + * pci_enable_link_state - Re-enable device's link state + * @pdev: PCI device + * @state: ASPM link states to re-enable + * + * Enable device's link state that were previously disable so the link is + * allowed to enter the specific states. Note that if the BIOS didn't grant + * ASPM control to the OS, this does nothing because we can't touch the + * LNKCTL register. + * + * Return: 0 or a negative errno. + */ +int pci_enable_link_state(struct pci_dev *pdev, int state) +{ + struct pcie_link_state *link = pcie_aspm_get_link(pdev); + + if (!link) + return -EINVAL; + /* + * A driver requested that ASPM be enabled on this device, but + * if we don't have permission to manage ASPM (e.g., on ACPI + * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and + * the _OSC method), we can't honor that request. + */ + if (aspm_disabled) { + pci_warn(pdev, "can't enable ASPM; OS doesn't have ASPM control\n"); + return -EPERM; + } + + mutex_lock(&aspm_lock); + link->aspm_disable &= ~pci_link_state_mask(state); + pcie_config_aspm_link(link, policy_to_aspm_state(link)); + + if (state & PCIE_LINK_STATE_CLKPM) + link->clkpm_disable = 0; + pcie_set_clkpm(link, policy_to_clkpm_state(link)); + mutex_unlock(&aspm_lock); + + return 0; +} +EXPORT_SYMBOL(pci_enable_link_state); + /** * pci_set_default_link_state - Set the default device link state * @pdev: PCI device diff --git a/include/linux/pci.h b/include/linux/pci.h index 3c24ca164104..844d09230264 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1776,11 +1776,13 @@ extern bool pcie_ports_native; int pci_disable_link_state(struct pci_dev *pdev, int state); int pci_disable_link_state_locked(struct pci_dev *pdev, int state); #ifdef CONFIG_PCIEASPM +int pci_enable_link_state(struct pci_dev *pdev, int state); int pci_set_default_link_state(struct pci_dev *pdev, int state); void pcie_no_aspm(void); bool pcie_aspm_support_enabled(void); bool pcie_aspm_enabled(struct pci_dev *pdev); #else +static inline int pci_enable_link_state(struct pci_dev *pdev, int state) { return -EOPNOTSUPP; } static inline int pci_set_default_link_state(struct pci_dev *pdev, int state) { return 0; } static inline void pcie_no_aspm(void) { }
pci_disable_link_state() lacks a symmetric pair. Some drivers want to disable ASPM during certain phases of their operation but then re-enable it later on. If pci_disable_link_state() is made for the device, there is currently no way to re-enable the states that were disabled. Add pci_enable_link_state() to remove ASPM states from the state disable mask. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- drivers/pci/pcie/aspm.c | 42 +++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 2 ++ 2 files changed, 44 insertions(+)