linux-kernel - Re: [PATCH v2 2/8] PCI/ASPM: Fix the behavior of pci_enable_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f644fc83-31cc-1f0e-58cf-7c007e6173e4@linux.intel.com>
Date: Tue, 26 Aug 2025 15:55:42 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, 
    David Box <david.e.box@...ux.intel.com>
cc: Bjorn Helgaas <bhelgaas@...gle.com>, 
    Manivannan Sadhasivam <mani@...nel.org>, 
    Lorenzo Pieralisi <lpieralisi@...nel.org>, 
    Krzysztof Wilczyński <kwilczynski@...nel.org>, 
    Rob Herring <robh@...nel.org>, Nirmal Patel <nirmal.patel@...ux.intel.com>, 
    Jonathan Derrick <jonathan.derrick@...ux.dev>, 
    Jeff Johnson <jjohnson@...nel.org>, linux-pci@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>, linux-arm-msm@...r.kernel.org, 
    linux-wireless@...r.kernel.org, ath12k@...ts.infradead.org, 
    ath11k@...ts.infradead.org, ath10k@...ts.infradead.org, 
    Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, 
    "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v2 2/8] PCI/ASPM: Fix the behavior of pci_enable_link_state*()
 APIs

+David

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

> From: Manivannan Sadhasivam <manivannan.sadhasivam@....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@...ux.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@....qualcomm.com>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....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)
>  {
> 
>