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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- 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)
>  {
> 
>