| 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
| ||
|
Message-ID: <c68046c7-24fc-4877-9493-3a40c952e2d0@oss.qualcomm.com>
Date: Mon, 25 Aug 2025 14:04:29 +0530
From: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
To: manivannan.sadhasivam@....qualcomm.com,
Manivannan Sadhasivam <mani@...nel.org>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kwilczynski@...nel.org>,
Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>
Cc: linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org,
"David E. Box" <david.e.box@...ux.intel.com>,
Johan Hovold <johan@...nel.org>, stable+noautosel@...nel.org
Subject: Re: [PATCH v3] PCI: qcom: Use pci_host_set_default_pcie_link_state()
API to enable ASPM for all platforms
On 8/25/2025 1:52 PM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
>
> Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0
> ops") allowed the Qcom controller driver to enable ASPM for all PCI devices
> enumerated at the time of the controller driver probe. It proved to be
> useful for devices already powered on by the bootloader, as it allowed
> devices to enter ASPM without user intervention.
>
> However, it could not enable ASPM for the hotplug capable devices i.e.,
> devices enumerated *after* the controller driver probe. This limitation
> mostly went unnoticed as the Qcom PCI controllers are not hotplug capable
> and also the bootloader has been enabling the PCI devices before Linux
> Kernel boots (mostly on the Qcom compute platforms which users use on a
> daily basis).
>
> But with the advent of the commit b458ff7e8176 ("PCI/pwrctl: Ensure that
> pwrctl drivers are probed before PCI client drivers"), the pwrctrl driver
> started to block the PCI device enumeration until it had been probed.
> Though, the intention of the commit was to avoid race between the pwrctrl
> driver and PCI client driver, it also meant that the pwrctrl controlled PCI
> devices may get probed after the controller driver and will no longer have
> ASPM enabled. So users started noticing high runtime power consumption with
> WLAN chipsets on Qcom compute platforms like Thinkpad X13s, and Thinkpad
> T14s, etc...
>
> Obviously, it is the pwrctrl change that caused regression, but it
> ultimately uncovered a flaw in the ASPM enablement logic of the controller
> driver. So to address the actual issue, use the newly introduced
> pci_host_set_default_pcie_link_state() API, which allows the controller
> drivers to set the default ASPM state for *all* devices. This default state
> will be honored by the ASPM driver while enabling ASPM for all the devices.
>
> So with this change, we can get rid of the controller driver specific
> 'qcom_pcie_ops::host_post_init' callback.
>
> Also with this change, ASPM is now enabled by default on all Qcom
> platforms as I haven't heard of any reported issues (apart from the
> unsupported L0s on some platorms, which is still handled separately).
>
> Cc: stable+noautosel@...nel.org # API dependency
> Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops")
> Reported-by: Johan Hovold <johan@...nel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
Reviewed-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
- Krishna Chaitanya.
> ---
> Changes in v3:
>
> - Moved the ASPM change to qcom_pcie_host_init()
> - Link to v2: https://lore.kernel.org/r/20250823-qcom_aspm_fix-v2-1-7cef4066663c@oss.qualcomm.com
>
> Changes in v2:
>
> * Used the new API introduced in this patch instead of bus notifier:
> https://lore.kernel.org/linux-pci/20250822031159.4005529-1-david.e.box@linux.intel.com/
>
> * Enabled ASPM on all platforms as there is no specific reason to limit it to a
> few.
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 34 ++--------------------------------
> 1 file changed, 2 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..0c6741b9a9585bd5566b23ba68ef12f1febab56b 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -247,7 +247,6 @@ struct qcom_pcie_ops {
> int (*get_resources)(struct qcom_pcie *pcie);
> int (*init)(struct qcom_pcie *pcie);
> int (*post_init)(struct qcom_pcie *pcie);
> - void (*host_post_init)(struct qcom_pcie *pcie);
> void (*deinit)(struct qcom_pcie *pcie);
> void (*ltssm_enable)(struct qcom_pcie *pcie);
> int (*config_sid)(struct qcom_pcie *pcie);
> @@ -1040,25 +1039,6 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
> return 0;
> }
>
> -static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
> -{
> - /*
> - * Downstream devices need to be in D0 state before enabling PCI PM
> - * substates.
> - */
> - pci_set_power_state_locked(pdev, PCI_D0);
> - pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> -
> - return 0;
> -}
> -
> -static void qcom_pcie_host_post_init_2_7_0(struct qcom_pcie *pcie)
> -{
> - struct dw_pcie_rp *pp = &pcie->pci->pp;
> -
> - pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_aspm, NULL);
> -}
> -
> static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> {
> struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> @@ -1336,6 +1316,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> goto err_assert_reset;
> }
>
> + pci_host_set_default_pcie_link_state(pp->bridge, PCIE_LINK_STATE_ALL);
> +
> return 0;
>
> err_assert_reset:
> @@ -1358,19 +1340,9 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
> pcie->cfg->ops->deinit(pcie);
> }
>
> -static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
> -{
> - struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> - struct qcom_pcie *pcie = to_qcom_pcie(pci);
> -
> - if (pcie->cfg->ops->host_post_init)
> - pcie->cfg->ops->host_post_init(pcie);
> -}
> -
> static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> .init = qcom_pcie_host_init,
> .deinit = qcom_pcie_host_deinit,
> - .post_init = qcom_pcie_host_post_init,
> };
>
> /* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
> @@ -1432,7 +1404,6 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> .get_resources = qcom_pcie_get_resources_2_7_0,
> .init = qcom_pcie_init_2_7_0,
> .post_init = qcom_pcie_post_init_2_7_0,
> - .host_post_init = qcom_pcie_host_post_init_2_7_0,
> .deinit = qcom_pcie_deinit_2_7_0,
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> .config_sid = qcom_pcie_config_sid_1_9_0,
> @@ -1443,7 +1414,6 @@ static const struct qcom_pcie_ops ops_1_21_0 = {
> .get_resources = qcom_pcie_get_resources_2_7_0,
> .init = qcom_pcie_init_2_7_0,
> .post_init = qcom_pcie_post_init_2_7_0,
> - .host_post_init = qcom_pcie_host_post_init_2_7_0,
> .deinit = qcom_pcie_deinit_2_7_0,
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> };
>
> ---
> base-commit: 681accdad91923ef4938b96ff3eea62e29af74c3
> change-id: 20250823-qcom_aspm_fix-3264926b5b14
>
> Best regards,
Powered by blists - more mailing lists