On Wed, Aug 27, 2025 at 10:47:39AM GMT, Bjorn Helgaas wrote:
> On Mon, Aug 25, 2025 at 01:52:57PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> >
> > 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 I guess this fixes a power consumption regression that became
> visible with b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are
> probed before PCI client drivers"). Arguably we should have a Fixes:
> tag for that, and maybe even skip the tag for 9f4f3dfad8cf, since if
> you have 9f4f3dfad8cf but not b458ff7e8176, it doesn't sound like you
> would need to backport this change?
>
9f4f3dfad8cf is the culprit which added a half baked solution for enabling ASPM
and b458ff7e8176 made the issue more obvious since instead of requiring people
to connect a device after boot, it allowed the device to enumerate after the
probe of the controller driver, thereby making it more visible.
It would make sense to add a Fixes tag for b458ff7e8176 too, but 9f4f3dfad8cf
should also be present IMO.
For backport, I do not want the AI tools to do the job since they will fail
anyway because of the API dependency. I may send a separate backport patch later
once this patch gets merged into mainline.
> I *love* getting rid of the bus walk and solving the hotplug issue.
>
> > 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).
>
> If ASPM hasn't been enabled by default, how would you hear about
> issues? Is ASPM commonly enabled in some other way?
>
Mostly from the downstream drivers. They do enable ASPM by default and I haven't
heard of any issues with that. So I assumed that would mean it will be safe for
us to enable ASPM for all platforms in upstream as well.
> If you think the risk of ASPM issues is nil, I guess it's OK to do at
> the same time. From a maintenance perspective it might be nice to
> separate that change so if there happened to be a regression, we could
> identify and revert that part by itself if necessary. It looks like
> previously, ASPM was only enabled for one part:
>
> ops_2_7_0 # cfg_2_7_0 qcom,pcie-sdm845
>
No. Previously ASPM was enabled for ops_1_9_0 and ops_1_21_0 based platforms.
> But after this patch, ASPM will be enabled for many more parts:
>
> ops_2_1_0 # cfg_2_1_0 qcom,pcie-apq8064 qcom,pcie-ipq8064 qcom,pcie-ipq8064-v2
> ops_1_0_0 # cfg_1_0_0 qcom,pcie-apq8084
> ops_2_3_2 # cfg_2_3_2 qcom,pcie-msm8996
> ops_2_4_0 # cfg_2_4_0 qcom,pcie-ipq4019 qcom,pcie-qcs404
> ops_2_3_3 # cfg_2_3_3 qcom,pcie-ipq8074
> ops_1_9_0 # cfg_1_9_0 qcom,pcie-sc7280 qcom,pcie-sc8180x qcom,pcie-sdx55 qcom,pcie-sm8150 qcom,pcie-sm8250 qcom,pcie-sm8350 qcom,pcie-sm8450-pcie0 qcom,pcie-sm8450-pcie1 qcom,pcie-sm8550
> # cfg_1_34_0 qcom,pcie-sa8775p
> ops_1_21_0 # cfg_sc8280xp qcom,pcie-sa8540p qcom,pcie-sc8280xp qcom,pcie-x1e80100
> ops_2_9_0 # cfg_2_9_0 qcom,pcie-ipq5018 qcom,pcie-ipq6018 qcom,pcie-ipq8074-gen3 qcom,pcie-ipq9574
>
If you insist, I can do that by calling pci_host_set_default_pcie_link_state()
from qcom_pcie_init_2_7_0() and later move it to qcom_pcie_host_init() in a
separate patch.
- Mani
--
மணிவண்ணன் சதாசிவம்
