On Mon, Jul 14, 2025 at 11:31:04PM +0530, Manivannan Sadhasivam wrote:
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, switch to the bus notifier for enabling ASPM of the PCI devices. The notifier will notify the controller driver when a PCI device is attached to the bus, thereby allowing it to enable ASPM more reliably. It should be noted that the 'pci_dev::link_state', which is required for enabling ASPM by the pci_enable_link_state_locked() API, is only set by the time of BUS_NOTIFY_BIND_DRIVER stage of the notification. So we cannot enable ASPM during BUS_NOTIFY_ADD_DEVICE stage.
A problem with this approach is that ASPM will never be enabled (and power consumption will be higher) in case an endpoint driver is missing.
I think that's something we should try to avoid.
So with this, we can also get rid of the controller driver specific 'qcom_pcie_ops::host_post_init' callback.
Cc: stable@vger.kernel.org # v6.7 Fixes: 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting 1.9.0 ops") Reported-by: Johan Hovold johan@kernel.org Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@oss.qualcomm.com
Note that the patch fails to apply to 6.16-rc6 due to changes in linux-next. Depending on how fast we can come up with a fix it may be better to target 6.16.
-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;
-}
I think you should consider leaving this helper in place here to keep the size of the diff down (e.g. as you intend to backport this).
+static int qcom_pcie_enable_aspm(struct pci_dev *pdev) +{
- /*
* 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);
You need to use the non-locked helpers here since you no longer hold the bus semaphore (e.g. as reported by lockdep).
Maybe this makes the previous comment about not moving the helper moot.
- return 0;
+}
+static int pcie_qcom_notify(struct notifier_block *nb, unsigned long action,
void *data)+{
- struct qcom_pcie *pcie = container_of(nb, struct qcom_pcie, nb);
This results in an unused variable warning (presumably until the next patch in the series is applied).
- struct device *dev = data;
- struct pci_dev *pdev = to_pci_dev(dev);
- switch (action) {
- case BUS_NOTIFY_BIND_DRIVER:
qcom_pcie_enable_aspm(pdev);break;- }
- return NOTIFY_DONE;
+}
Missing newline.
static int qcom_pcie_probe(struct platform_device *pdev) { const struct qcom_pcie_cfg *pcie_cfg;
Johan