Re: [PATCH v2 2/8] PCI/ASPM: Fix the behavior of pci_enable_link_state*() APIs

Ilpo Järvinen
2025-08-26T13:02:22Z

[-- Attachment #1: Type: text/plain, Size: 5816 bytes --]

+David

On Mon, 25 Aug 2025, Manivannan Sadhasivam via B4 Relay wrote:

> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> pci_enable_link_state() and pci_enable_link_state_locked() APIs are
> supposed to be symmectric with pci_disable_link_state() and
> pci_disable_link_state_locked() APIs.
>
> But unfortunately, they are not symmetric. This behavior was mentioned in
> the kernel-doc of these APIs:
>
> " Clear and set the default device link state..."
>
> and
>
> "Also note that this does not enable states disabled by
> pci_disable_link_state()"
>
> These APIs won't enable all the states specified by the 'state' parameter,
> but only enable the ones not previously disabled by the
> pci_disable_link_state*() APIs. But this behavior doesn't align with the
> naming of these APIs, as they give the impression that these APIs will
> enable all the specified states.
>
> To resolve this ambiguity, allow these APIs to enable the specified states,
> regardeless of whether they were previously disabled or not. This is
> accomplished by clearing the previously disabled states from the
> 'link::aspm_disable' parameter in __pci_enable_link_state() helper. Also,
> reword the kernel-doc to reflect this behavior.
>
> The current callers of pci_enable_link_state_locked() APIs (vmd and
> pcie-qcom) did not disable the ASPM states before calling this API. So it
> is evident that they do not depend on the previous behavior of this API and
> intend to enable all the specified states.

While it might be "safe" in the sense that ->aspm_disable is not set by
anything, I'm still not sure if overloading this function for two
different use cases is a good idea.

I'd like to hear David's opinion on this as he grasps the ->aspm_default
vs ->aspm_disable thing much better than I do.

> And the other API, pci_enable_link_state() doesn't have a caller for now,
> but will be used by the 'atheros' WLAN drivers in the subsequent commits.
>
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

This tag sound like I'm endorsing this approach which is not the case. I'd
prefer separate functions for each use case, setting aspm_default and
another for the enable state.

--
i.

> Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
> drivers/pci/pcie/aspm.c | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index be9bd272057c3472f3e31dc9568340b19d52012a..fac46113a90c7fac6c97125e6a7e385045780005 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1459,6 +1459,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
> down_read(&pci_bus_sem);
> mutex_lock(&aspm_lock);
> link->aspm_default = pci_calc_aspm_enable_mask(state);
> + link->aspm_disable &= ~state;
> pcie_config_aspm_link(link, policy_to_aspm_state(link));
>
> link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> @@ -1471,17 +1472,18 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
> }
>
> /**
> - * pci_enable_link_state - Clear and set the default device link state so that
> - * the link may be allowed to enter the specified 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. Also note that this does not enable states
> - * disabled by pci_disable_link_state(). Return 0 or a negative errno.
> + * pci_enable_link_state - Enable device's link state
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + *
> + * Enable device's link state, so the link will enter the specified 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.
> *
> * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
> * PCIe r6.0, sec 5.5.4.
> *
> - * @pdev: PCI device
> - * @state: Mask of ASPM link states to enable
> + * Return: 0 on success, a negative errno otherwise.
> */
> int pci_enable_link_state(struct pci_dev *pdev, int state)
> {
> @@ -1490,19 +1492,20 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
> EXPORT_SYMBOL(pci_enable_link_state);
>
> /**
> - * pci_enable_link_state_locked - Clear and set the default device link state
> - * so that the link may be allowed to enter the specified 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. Also note that this does not enable states
> - * disabled by pci_disable_link_state(). Return 0 or a negative errno.
> + * pci_enable_link_state_locked - Enable device's link state
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + *
> + * Enable device's link state, so the link will enter the specified 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.
> *
> * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
> * PCIe r6.0, sec 5.5.4.
> *
> - * @pdev: PCI device
> - * @state: Mask of ASPM link states to enable
> - *
> * Context: Caller holds pci_bus_sem read lock.
> + *
> + * Return: 0 on success, a negative errno otherwise.
> */
> int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
> {
>
>